Re: [PATCH] libsubcmd: Fix null intersection case in exclude_cmds()

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Mon, Dec 08, 2025 at 09:26:49AM -0800, Ian Rogers wrote:
> On Sun, Dec 7, 2025 at 2:16 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Tue, Dec 2, 2025 at 1:40 PM Sri Jayaramappa <sjayaram@akamai.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@akamai.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@google.com>
> 
> I posted the test as a patch here:
> https://lore.kernel.org/lkml/20251208172339.1445817-1-irogers@google.com/

So, finally this one is also applied, together with Guilherme's
Tested-by/Reviewed-by tags, etc.

- Arnaldo