Re: [PATCH v2 26/26] perf hist: Fix srcline memory leak

[Andi Kleen]

On Mon, Jun 12, 2023 at 02:23:41PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 12, 2023 at 07:46:14AM -0700, Ian Rogers escreveu:
> > On Mon, Jun 12, 2023 at 7:16 AM Arnaldo Carvalho de Melo
> > <acme@xxxxxxxxxx> wrote:
> > >
> > > Em Mon, Jun 12, 2023 at 11:13:59AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Thu, Jun 08, 2023 at 04:28:23PM -0700, Ian Rogers escreveu:
> > > > > srcline isn't freed if it is SRCLINE_UNKNOWN. Avoid strduping in this
> > > > > case as such strdups are redundant and leak memory.
> > > >
> > > > The patch is ok as its what the rest of the code is doing, i.e. strcmp()
> > > > to check if a srcline is the unknown one, but how about the following
> > > > patch on top of yours?
> > >
> > > [acme@quaco perf-tools-next]$ strings ~/bin/perf | grep '??:0'
> > > ??:0
> > > SRCLINE_UNKNOWN ((char *) "??:0")
> > > [acme@quaco perf-tools-next]$
> > 
> > Agreed, the strcmps make me nervous as they won't distinguish heap
> > from a global meaning we could end up with things like pointers to
> > freed memory. The comparison with the global is always going to be
> > same imo.
> > 
> > Acked-by: Ian Rogers <irogers@xxxxxxxxxx>
> 
> Thanks, applied and added your Acked-by.

Actually was there another patch that turned it into a explicit global? 

At least in my tree it isn't:

util/srcline.h
28:#define SRCLINE_UNKNOWN  ((char *) "??:0")

Without any explicit global it's a bit dangerous because you rely on the
linker doing string deduplication.  While it normally does that there might be
odd corner cases where it doesn't.

-Andi