Message-ID: <CAP-5=fU8PA0u=YKroNK8ykF3f8oUrttgNzJ13LELCmNL0JO+=Q@mail.gmail.com>
Date: Mon, 8 Dec 2025 09:26:49 -0800
From: Ian Rogers <irogers@...gle.com>
To: Sri Jayaramappa <sjayaram@...mai.com>, Guilherme Amadio <amadio@...too.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Joshua Hunt <johunt@...mai.com>
Subject: Re: [PATCH] libsubcmd: Fix null intersection case in exclude_cmds()

On Sun, Dec 7, 2025 at 2:16 PM Ian Rogers <irogers@...gle.com> wrote:
>
> On Tue, Dec 2, 2025 at 1:40 PM Sri Jayaramappa <sjayaram@...mai.com> wrote:
> >
> > When there is no exclusion occurring from the cmds list - for example -
> > cmds contains ["read-vdso32"] and excludes contains ["archive"] - the
> > main loop completes with ci == cj == 0. In the original code the loop
> > processing the remaining elements in the list was conditional:
> >
> >     if (ci != cj) { ...}
> >
> > So we end up in the assertion loop since ci < cmds->cnt and we
> > incorrectly try to assert the list elements to be NULL and fail with
> > the following error
> >
> >    help.c:104: exclude_cmds: Assertion `cmds->names[ci] == NULL' failed.
> >
> > Fix this by moving the if (ci != cj) check inside of a broader loop.
> > If ci != cj, left shift the list elements, as before, and then
> > unconditionally advance the ci and cj indicies which also covers the
> > ci == cj case.
> >
> > Fixes: 1fdf938168c4d26f ("perf tools: Fix use-after-free in help_unknown_cmd()")
> >
> > Signed-off-by: Sri Jayaramappa <sjayaram@...mai.com>
>
> Thanks Sri! Guilherme reported a related issue and I think your fix covers it:
> https://lore.kernel.org/lkml/aTXQw9dtP5df7LmP@gentoo.org/
>
> I came up with the following test based on your commit message, could
> you validate it covers the case for your fix:
> ```
> diff --git a/tools/perf/tests/subcmd-help.c b/tools/perf/tests/subcmd-help.c
> index 2280b4c0e5e7..9da96a16fd20 100644
> --- a/tools/perf/tests/subcmd-help.c
> +++ b/tools/perf/tests/subcmd-help.c
> @@ -95,10 +95,36 @@ static int test__exclude_cmdnames(struct
> test_suite *test __maybe_unused,
>        return TEST_OK;
> }
>
> +static int test__exclude_cmdnames_no_overlap(struct test_suite *test
> __maybe_unused,
> +                                            int subtest __maybe_unused)
> +{
> +       struct cmdnames cmds1 = {};
> +       struct cmdnames cmds2 = {};
> +
> +       add_cmdname(&cmds1, "read-vdso32", 11);
> +       add_cmdname(&cmds2, "archive", 7);
> +
> +       TEST_ASSERT_VAL("invalid original size", cmds1.cnt == 1);
> +       TEST_ASSERT_VAL("invalid original size", cmds2.cnt == 1);
> +
> +       exclude_cmds(&cmds1, &cmds2);
> +
> +       TEST_ASSERT_VAL("invalid excluded size", cmds1.cnt == 1);
> +       TEST_ASSERT_VAL("invalid excluded size", cmds2.cnt == 1);
> +
> +       TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1,
> "read-vdso32") == 1);
> +       TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "archive") == 0);
> +
> +       clean_cmdnames(&cmds1);
> +       clean_cmdnames(&cmds2);
> +       return TEST_OK;
> +}
> +
> static struct test_case tests__subcmd_help[] = {
>        TEST_CASE("Load subcmd names", load_cmdnames),
>        TEST_CASE("Uniquify subcmd names", uniq_cmdnames),
>        TEST_CASE("Exclude duplicate subcmd names", exclude_cmdnames),
> +       TEST_CASE("Exclude disjoint subcmd names", exclude_cmdnames_no_overlap),
>        {       .name = NULL, }
> };
> ```
> If you build perf you can run the test like:
> ```
> $ perf test -v help
>  68: libsubcmd help tests                                            :
>  68.1: Load subcmd names                                             : Ok
>  68.2: Uniquify subcmd names                                         : Ok
>  68.3: Exclude duplicate subcmd names                                : Ok
>  68.4: Exclude disjoint subcmd names                                 : Ok
> ```
> Perhaps you can think of other values for the test so we don't run
> into more issues.

Running the test above I get the following before:
```
 68.4: Exclude disjoint subcmd names:
--- start ---
test child forked, pid 1443868
perf: help.c:107: exclude_cmds: Assertion `cmds->names[ci] == NULL' failed.

---- unexpected signal (6) ----
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
Failed to read build ID for //anon
    #0 0x55ed35f172de in child_test_sig_handler builtin-test.c:312
    #1 0x7fea1d049df0 in __restore_rt libc_sigaction.c:0
    #2 0x7fea1d09e95c in __pthread_kill_implementation pthread_kill.c:44
    #3 0x7fea1d049cc2 in raise raise.c:27
    #4 0x7fea1d0324ac in abort abort.c:81
    #5 0x7fea1d032420 in __assert_perror_fail assert-perr.c:31
    #6 0x55ed35ea1524 in exclude_cmds help.c:106
    #7 0x55ed35f62e82 in test__exclude_cmdnames_no_overlap subcmd-help.c:112
    #8 0x55ed35f17470 in run_test_child builtin-test.c:340
    #9 0x55ed35ea5834 in start_command run-command.c:128
    #10 0x55ed35f17ee3 in start_test builtin-test.c:546
    #11 0x55ed35f1838d in __cmd_test builtin-test.c:648
    #12 0x55ed35f18f2d in cmd_test builtin-test.c:850
    #13 0x55ed35e94494 in run_builtin perf.c:349
    #14 0x55ed35e9472c in handle_internal_command perf.c:401
    #15 0x55ed35e94885 in run_argv perf.c:448
    #16 0x55ed35e94bce in main perf.c:555
    #17 0x7fea1d033ca8 in __libc_start_call_main libc_start_call_main.h:74
    #18 0x7fea1d033d65 in __libc_start_main@@GLIBC_2.34 libc-start.c:128
    #19 0x55ed35de4f41 in _start perf[52f41]
 68.4: Exclude disjoint subcmd names                                 : FAILED!
```
After the test passes and is address/leak sanitizer clean.

Tested-by: Ian Rogers <irogers@...gle.com>

I posted the test as a patch here:
https://lore.kernel.org/lkml/20251208172339.1445817-1-irogers@google.com/

Thanks,
Ian
> Thanks,
> Ian
>
> > ---
> >  tools/lib/subcmd/help.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/subcmd/help.c b/tools/lib/subcmd/help.c
> > index ddaeb4eb3e24..db94aa685b73 100644
> > --- a/tools/lib/subcmd/help.c
> > +++ b/tools/lib/subcmd/help.c
> > @@ -97,11 +97,13 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
> >                         ei++;
> >                 }
> >         }
> > -       if (ci != cj) {
> > -               while (ci < cmds->cnt) {
> > -                       cmds->names[cj++] = cmds->names[ci];
> > -                       cmds->names[ci++] = NULL;
> > +       while (ci < cmds->cnt) {
> > +               if (ci != cj) {
> > +                       cmds->names[cj] = cmds->names[ci];
> > +                       cmds->names[ci] = NULL;
> >                 }
> > +               ci++;
> > +               cj++;
> >         }
> >         for (ci = cj; ci < cmds->cnt; ci++)
> >                 assert(cmds->names[ci] == NULL);
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists