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

[Arnaldo Carvalho de Melo]

Em Mon, Jun 12, 2023 at 02:16:34PM -0700, Andi Kleen escreveu:
> 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.

Yeah, several gcc and clang versions complained with:

2    65.38 almalinux:9                   : FAIL clang version 15.0.7 (Red Hat 15.0.7-2.el9)
    builtin-diff.c:1381:17: error: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Werror,-Wstring-compare]
            if (start_line != SRCLINE_UNKNOWN &&

So I added the following on top:

diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 48a04f42b308b080..aec596a0b0bbec0f 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -23,6 +23,8 @@
 
 bool srcline_full_filename;
 
+char *srcline__unknown = (char *)"??:0";
+
 static const char *dso__name(struct dso *dso)
 {
 	const char *dso_name;
diff --git a/tools/perf/util/srcline.h b/tools/perf/util/srcline.h
index a15c7db9058ece96..167645bcff0755e2 100644
--- a/tools/perf/util/srcline.h
+++ b/tools/perf/util/srcline.h
@@ -25,7 +25,8 @@ char *srcline__tree_find(struct rb_root_cached *tree, u64 addr);
 /* delete all srclines within the tree */
 void srcline__tree_delete(struct rb_root_cached *tree);
 
-#define SRCLINE_UNKNOWN  ((char *) "??:0")
+extern char *srcline__unknown;
+#define SRCLINE_UNKNOWN srcline__unknown
 
 struct inline_list {
 	struct symbol		*symbol;