From: Michael Petlan <mpetlan@redhat.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: acme@redhat.com, linux-perf-users@vger.kernel.org,
jolsa@redhat.com, jlelli@redhat.com, milian.wolff@kdab.com
Subject: Re: [PATCH] perf machine: Initialize srcline string member in add_location struct
Date: Mon, 13 Sep 2021 17:13:12 +0200 (CEST) [thread overview]
Message-ID: <alpine.LRH.2.20.2109131651090.27231@Diego> (raw)
In-Reply-To: <YToIuTUuudHKZmN2@kernel.org>
On Thu, 9 Sep 2021, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 19, 2021 at 04:53:32PM +0200, Michael Petlan escreveu:
> > It's later supposed to be either a correct address or NULL. Without the
> > initialization, it may contain an undefined value which results in the
> > following segmentation fault:
> >
> > # perf top --sort comm -g --ignore-callees=do_idle
>
> Later where? The backtrace below is unresolved, I couldn't reproduce it
> here, that al variable is local to add_callchain_ip(), and is then
> passed to several places where I couldn't find al->srcline being used,
> can you state where it is used and causes the segfault?
>
Here is a better backtrace of a segfault:
#0 0x00007ffff56b7685 in __strlen_avx2 () from /lib64/libc.so.6
#1 0x00007ffff55e3802 in strdup () from /lib64/libc.so.6
#2 0x00005555558cb139 in hist_entry__init (callchain_size=<optimized out>, sample_self=true, template=0x7fffde7fb110, he=0x7fffd801c250) at util/hist.c:489
#3 hist_entry__new (template=template@entry=0x7fffde7fb110, sample_self=sample_self@entry=true) at util/hist.c:564
#4 0x00005555558cb4ba in hists__findnew_entry (hists=hists@entry=0x5555561d9e38, entry=entry@entry=0x7fffde7fb110, al=al@entry=0x7fffde7fb420,
sample_self=sample_self@entry=true) at util/hist.c:657
#5 0x00005555558cba1b in __hists__add_entry (hists=hists@entry=0x5555561d9e38, al=0x7fffde7fb420, sym_parent=<optimized out>, bi=bi@entry=0x0, mi=mi@entry=0x0,
sample=sample@entry=0x7fffde7fb4b0, sample_self=true, ops=0x0, block_info=0x0) at util/hist.c:288
#6 0x00005555558cbb70 in hists__add_entry (sample_self=true, sample=0x7fffde7fb4b0, mi=0x0, bi=0x0, sym_parent=<optimized out>, al=<optimized out>, hists=0x5555561d9e38)
at util/hist.c:1056
#7 iter_add_single_cumulative_entry (iter=0x7fffde7fb460, al=<optimized out>) at util/hist.c:1056
#8 0x00005555558cc8a4 in hist_entry_iter__add (iter=iter@entry=0x7fffde7fb460, al=al@entry=0x7fffde7fb420, max_stack_depth=<optimized out>, arg=arg@entry=0x7fffffff7db0)
at util/hist.c:1231
#9 0x00005555557cdc9a in perf_event__process_sample (machine=<optimized out>, sample=0x7fffde7fb4b0, evsel=<optimized out>, event=<optimized out>, tool=0x7fffffff7db0)
at builtin-top.c:842
#10 deliver_event (qe=<optimized out>, qevent=<optimized out>) at builtin-top.c:1202
#11 0x00005555558a9318 in do_flush (show_progress=false, oe=0x7fffffff80e0) at util/ordered-events.c:244
#12 __ordered_events__flush (oe=oe@entry=0x7fffffff80e0, how=how@entry=OE_FLUSH__TOP, timestamp=timestamp@entry=0) at util/ordered-events.c:323
#13 0x00005555558a9789 in __ordered_events__flush (timestamp=<optimized out>, how=<optimized out>, oe=<optimized out>) at util/ordered-events.c:339
#14 ordered_events__flush (how=OE_FLUSH__TOP, oe=0x7fffffff80e0) at util/ordered-events.c:341
#15 ordered_events__flush (oe=oe@entry=0x7fffffff80e0, how=how@entry=OE_FLUSH__TOP) at util/ordered-events.c:339
#16 0x00005555557cd631 in process_thread (arg=0x7fffffff7db0) at builtin-top.c:1114
#17 0x00007ffff7bb817a in start_thread () from /lib64/libpthread.so.0
#18 0x00007ffff5656dc3 in clone () from /lib64/libc.so.6
If you look at the frame #2, the code is:
488 if (he->srcline) {
489 he->srcline = strdup(he->srcline);
490 if (he->srcline == NULL)
491 goto err_rawdata;
492 }
If he->srcline is not NULL (it is not NULL if it is uninitialized rubbish),
it gets strdupped and strdupping a rubbish random string causes the problem.
Also, if you look at the commit 1fb7d06a509e, it adds the srcline property
into the struct, but not initializing it everywhere needed.
Michael
> - Arnaldo
>
> > terminates with
> >
> > perf: Segmentation fault
> > -------- backtrace --------
> > perf(+0x417b26)[0x557794f1fb26]
> > /lib64/libc.so.6(+0x37400)[0x7f62a0194400]
> > /lib64/libc.so.6(+0x15d685)[0x7f62a02ba685]
> > /lib64/libc.so.6(__strdup+0x12)[0x7f62a01e6802]
> > perf(+0x3769d9)[0x557794e7e9d9]
> > perf(+0x376d3a)[0x557794e7ed3a]
> > perf(+0x377284)[0x557794e7f284]
> > perf(+0x3773e0)[0x557794e7f3e0]
> > perf(hist_entry_iter__add+0xc4)[0x557794e80114]
> > perf(+0x2799aa)[0x557794d819aa]
> > perf(+0x354ec8)[0x557794e5cec8]
> > perf(+0x279341)[0x557794d81341]
> > /lib64/libpthread.so.0(+0x814a)[0x7f62a27b514a]
> > /lib64/libc.so.6(clone+0x43)[0x7f62a0259dc3]
> >
> > Fixes: 1fb7d06a509e ("perf report: Use srcline from callchain for hist entries")
> >
> > Signed-off-by: Michael Petlan <mpetlan@redhat.com>
> > Reported-by: Juri Lelli <jlelli@redhat.com>
> > CC: Milian Wolff <milian.wolff@kdab.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> > tools/perf/util/machine.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index da19be7da284..44e40bad0e33 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -2149,6 +2149,7 @@ static int add_callchain_ip(struct thread *thread,
> >
> > al.filtered = 0;
> > al.sym = NULL;
> > + al.srcline = NULL;
> > if (!cpumode) {
> > thread__find_cpumode_addr_location(thread, ip, &al);
> > } else {
> > --
> > 2.18.4
> >
>
> --
>
> - Arnaldo
>
>
next prev parent reply other threads:[~2021-09-13 15:13 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-19 14:53 [PATCH] perf machine: Initialize srcline string member in add_location struct Michael Petlan
2021-09-06 14:21 ` Michael Petlan
2021-09-09 13:14 ` Arnaldo Carvalho de Melo
2021-09-13 15:13 ` Michael Petlan [this message]
2021-09-14 21:21 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LRH.2.20.2109131651090.27231@Diego \
--to=mpetlan@redhat.com \
--cc=acme@kernel.org \
--cc=acme@redhat.com \
--cc=jlelli@redhat.com \
--cc=jolsa@redhat.com \
--cc=linux-perf-users@vger.kernel.org \
--cc=milian.wolff@kdab.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.