Hi Rob,
On Thu, Apr 25, 2019 at 01:43:02PM +0100, Robert Walker wrote:
Hi,
I've noticed that recent versions of perf will crash when processing CoreSight trace with some combinations of decoding options.
The crash occurs when neither the 'b' (branch events), or 'l' flag (last branch entries) options to --itrace are specified for the perf report/script/inject commands - e.g. 'perf report --itrace=i1000i'. The default flags includes 'b', so the crash doesn't occur if no '--itrace' option is given.
The crash appears to be due to the prev_packet field of cs_etm_queue (tools/perf/util/cs-etm.c) not being initialised unless either 'b' or 'l' is included in the trace flags. There seems to have been a number of uses of this field added without checking whether it's valid - these have been added in 5.0 (e.g. commits 7100b12: perf cs-etm: Generate branch sample for exception packet, 24fff5e: perf cs-etm: Avoid stale branch samples when flush packet) and there have been more added for 5.1-rc1: (06220bf: perf cs-etm: Set sample flags for instruction range packet and related commits)
As this field is used more widely now and it's already hard to follow which functions have been called in a context where the validity of prev_packet has been checked (i.e. "if (etm->synth_opts.last_branch || etm->sample_branches) { ... }", I think it would be better to create prev_packet in all cases.
After reading the code and added some logs, agree with you.
If we use the command 'perf report --itrace=i1000i', then the both flags 'etm->synth_opts.last_branch' and 'etm->sample_branches' will not be set, thus the function cs_etm__alloc_queue() skips to allocate one temporary data structure for 'etmq->prev_packet' in this case, but it causes the segment fault when invoke function cs_etm__exception().
So we can simply allocate data for 'etmq->prev_packet' from the beginning (in cs_etm__alloc_queue()), thus can avoid segment fault if access 'etmq->prev_packet'; after this change, since 'etmq->prev_packet' will always point to one data structure, I also suggest to remove all redundant code for checking 'etmq->prev_packet' is NULL pointer.
Thanks, Leo Yan