Re: [PATCH 4/4] perf symbol: Add missing libgen.h include to get basename() prototype

[Ian Rogers]

On Tue, Apr 7, 2026 at 5:08 AM Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
>
> On Thu, Apr 02, 2026 at 08:19:24AM -0700, Ian Rogers wrote:
> > On Wed, Apr 1, 2026 at 5:20 PM Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
> > >
> > > From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > >
> > > This works in some places because there are tricks to prefer a GNU
> > > version instead of the XPG version, but started breaking in some musl
> > > libc systems due to some unrelated change that made libgen.h stop being
> > > included by chance.
> > >
> > > Since symbol.h is using basename(), make it include libgen.h.
> >
> > Huh, this is being flagged by Sashiko:
> > """
> > Could this include cause segmentation faults by exposing the POSIX version
> > of basename() to the rest of the tools?
> >
> > According to the POSIX specification, if the input is an empty string
> > or slashes, basename() can return a pointer to a statically allocated,
> > read-only string literal (such as "." or "/").
> >
> > In tools/perf/util/probe-event.c:convert_exec_to_group(), the code takes
> > the return value of basename() and modifies it in-place:
> >     ptr = basename(exec);
> >     ...
> >     for (ptr2 = ptr; *ptr2 != '\0'; ptr2++) {
> >             if (!isalnum(*ptr2) && *ptr2 != '_')
> >                     *ptr2 = '_';
> >     }
> > If a user provides an empty target or "/", the POSIX basename() would
> > return a read-only string. When the loop attempts to write a null or
> > underscore into it, wouldn't it trigger an immediate segmentation fault?
> >
> > Is there a way we can safely duplicate the returned string before modifying it,
> > or ensure the inputs never resolve to these static strings?
> > """
> >
> > The story of the two basename versions and the issues appears in the
> > man pages. The whole thing makes me think we should avoid basename :-)
>
> What sashiko pointed out is a problem in some other part of perf that is
> unrelated to this patch and requires a separate fix, no?

Not quite, from the man page:

  With glibc, one gets the POSIX version of basename() when <libgen.h>
is included, and the GNU version otherwise.

As `symbol.h` is included in many places, then, with your change
whenever `symbol.h` is used the behavior of basename will flip.

> I.e. tools/perf/util/probe-event.c already includes libgen.h since
> 581037151910126a ("perf probe: Add missing libgen.h header needed for
> using basename()") from March, 2024.
>
> In the scope of this patch the potential problem isn't possible as
> __symbol__join_symfs() isn't changing the return of basename(), right?
>
> ⬢ [acme@toolbx perf-tools-next]$ git grep '\<basename(' tools/perf
> tools/perf/builtin-daemon.c:    base = basename(basen);
> tools/perf/scripts/python/exported-sql-viewer.py:               if short_name[0:7] == "[kernel" and os.path.basename(long_name) == "kcore":
> tools/perf/util/annotate.c:             d_filename = basename(filename);
> tools/perf/util/dsos.c:        * basename() may modify path buffer, so we must pass
> tools/perf/util/dsos.c:          * basename() may return a pointer to internal
> tools/perf/util/dsos.c:         base = strdup(basename(lname));
> tools/perf/util/probe-event.c:  ptr1 = basename(exec_copy);
> tools/perf/util/symbol.h:               base = basename(path_copy);
> ⬢ [acme@toolbx perf-tools-next]$
>
> It seems just the probe-event.c case needs fixing.
>
> And we also have this in tools/perf/util/srcline.c:
>
> /* basename version that takes a const input string */
> static const char *gnu_basename(const char *path)
> {
>         const char *base = strrchr(path, '/');
>
>         return base ? base + 1 : path;
> }
>
> That probably could be reused to avoid these basename() oddities?

Yeah, probably move gnu_basename to util.c/h and then make all callers
use gnu_basename and not basename. This would mean that symbol.h using
util.h, instead could we move  __symbol__join_symfs to symbol.c to
avoid all these includes and touching a file requiring everything
being rebuilt? The function is large enough that including it in the
header doesn't make much sense.

Thanks,
Ian

> - Arnaldo