From: Ian Rogers <irogers@google.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Namhyung Kim <namhyung@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Kan Liang <kan.liang@linux.intel.com>,
Oliver Upton <oliver.upton@linux.dev>,
James Clark <james.clark@arm.com>,
Tim Chen <tim.c.chen@linux.intel.com>,
Yicong Yang <yangyicong@hisilicon.com>,
K Prateek Nayak <kprateek.nayak@amd.com>,
Yanteng Si <siyanteng@loongson.cn>,
Sun Haiyong <sunhaiyong@loongson.cn>,
Kajol Jain <kjain@linux.ibm.com>,
Ravi Bangoria <ravi.bangoria@amd.com>, Li Dong <lidong@vivo.com>,
Paran Lee <p4ranlee@gmail.com>, Ben Gainey <ben.gainey@arm.com>,
Andi Kleen <ak@linux.intel.com>,
Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-perf-users <linux-perf-users@vger.kernel.org>
Subject: Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory
Date: Tue, 7 May 2024 18:17:47 -0700 [thread overview]
Message-ID: <CAP-5=fU1E4xY-DEpSh0ThGj+y4hmGHD9sMF_FPFBpAUuK6Pt3Q@mail.gmail.com> (raw)
In-Reply-To: <CA+JHD90W7PLBx=SEL9+7-_=LkjaMu4YM1S3kJ2oSkAYoHE7hPw@mail.gmail.com>
On Tue, May 7, 2024 at 6:07 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On Tue, May 7, 2024, 9:51 PM Ian Rogers <irogers@google.com> wrote:
>>
>> On Tue, May 7, 2024 at 2:07 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>> >
>> > On Tue, May 07, 2024 at 06:04:43PM -0300, Arnaldo Carvalho de Melo wrote:
>> > > On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
>> > > > On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>> > > > >
>> > > > > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
>> > > > > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
>> > > > > > > ui_browser__show is capturing the input title that is stack allocated
>> > > > > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
>> > > > > > > the string.
>> > > > > >
>> > > > > > But everything happens in that context, i.e. hist_brower__run() will
>> > > > > > call ui_browser__ methods and then exit.
>> > > > > >
>> > > > > > We end up having browser->title pointing to returned stack memory
>> > > > > > (invalid) but there will be no references to it, no?
>> > > > > >
>> > > > > > If we return to hist_browser__run() we then call ui_browser__show
>> > > > > > passing a new title, for "live" stack memory, rinse repeat. Or have you
>> > > > > > noticed an actual use-after-"free"?
>> > > > >
>> > > > > And I'll take the patch, I'm just trying to figure it out if it fixed a
>> > > > > real bug or if it just makes the code more future proof, i.e. to avoid
>> > > > > us adding code that actually uses invalid stack memory.
>> > > >
>> > > > My command line using tui is:
>> > > > $ sudo bash -c 'rm /tmp/asan.log*; export
>> > > > ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
>> > > > sleep 1; /tmp/perf/perf mem report'
>> > > > I then go to the perf annotate view and quit. This triggers the asan
>> > > > error (from the log file):
>> > > > ```
>> > >
>> > > Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
>> > > in the full details can hopefully find this message going from the Link:
>> > > tag.
>> >
>> > Nah, I added your explanation to the cset log message.
>>
>>
>> Okay, I found I needed this to avoid a segv introduced by this patch:
>> ```
>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>> index c4cdf2ea69b7..19503e838738 100644
>> --- a/tools/perf/ui/browser.c
>> +++ b/tools/perf/ui/browser.c
>> @@ -203,7 +203,7 @@ void ui_browser__refresh_dimensions(struct
>> ui_browser *browser)
>> void ui_browser__handle_resize(struct ui_browser *browser)
>> {
>> ui__refresh_dimensions(false);
>> - ui_browser__show(browser, browser->title, ui_helpline__current);
>> + ui_browser__show(browser, browser->title ?: "", ui_helpline__current);
>> ui_browser__refresh(browser);
>> }
>> ```
>> I also found a use-after-free issue with patch 5. I'll send a v2.
>>
>> Please send a fix, it's already in perf-tools-next.
>
Okay. It looks like you accidentally pushed tmp.perf-tools.next, that
is a .next rather than a -next (dot not dash). I'll work from
perf-tools-next.
Thanks,
Ian
> Thanks,
>
> - Arnaldo
>
>> Thanks,
>> Ian
>>
>> > Thanks,
>> >
>> > - Arnaldo
next prev parent reply other threads:[~2024-05-08 1:18 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 18:35 [PATCH v1 0/8] Address/leak sanitizer clean ups Ian Rogers
2024-05-07 18:35 ` [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory Ian Rogers
2024-05-07 20:20 ` Arnaldo Carvalho de Melo
2024-05-07 20:22 ` Arnaldo Carvalho de Melo
2024-05-07 20:48 ` Ian Rogers
2024-05-07 21:04 ` Arnaldo Carvalho de Melo
2024-05-07 21:07 ` Arnaldo Carvalho de Melo
2024-05-08 0:51 ` Ian Rogers
[not found] ` <CA+JHD90W7PLBx=SEL9+7-_=LkjaMu4YM1S3kJ2oSkAYoHE7hPw@mail.gmail.com>
2024-05-08 1:17 ` Ian Rogers [this message]
2024-05-07 18:35 ` [PATCH v1 2/8] perf annotate: Fix memory leak in annotated_source Ian Rogers
2024-05-07 18:35 ` [PATCH v1 3/8] perf block-info: Remove unused refcount Ian Rogers
2024-05-07 18:35 ` [PATCH v1 4/8] perf cpumap: Remove refcnt from cpu_aggr_map Ian Rogers
2024-05-07 18:35 ` [PATCH v1 5/8] perf comm: Add reference count checking to comm_str Ian Rogers
2024-05-07 18:35 ` [PATCH v1 6/8] perf mem-info: Move mem-info out of mem-events and symbol Ian Rogers
2024-05-07 18:35 ` [PATCH v1 7/8] perf mem-info: Add reference count checking Ian Rogers
2024-05-07 18:35 ` [PATCH v1 8/8] perf hist: Avoid hist_entry_iter mem_info memory leak Ian Rogers
2024-05-07 20:51 ` [PATCH v1 0/8] Address/leak sanitizer clean ups 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='CAP-5=fU1E4xY-DEpSh0ThGj+y4hmGHD9sMF_FPFBpAUuK6Pt3Q@mail.gmail.com' \
--to=irogers@google.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=arnaldo.melo@gmail.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=ben.gainey@arm.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=kjain@linux.ibm.com \
--cc=kprateek.nayak@amd.com \
--cc=lidong@vivo.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=p4ranlee@gmail.com \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@amd.com \
--cc=siyanteng@loongson.cn \
--cc=sunhaiyong@loongson.cn \
--cc=tim.c.chen@linux.intel.com \
--cc=yangyicong@hisilicon.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).