Re: [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd()

[Ian Rogers]

On Mon, Jun 30, 2025 at 4:32 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> Currently perf aborts when it finds an invalid command.  I guess it
> depends on the environment as I have some custom commands in the path.
>
>   $ perf bad-command
>   perf: 'bad-command' is not a perf-command. See 'perf --help'.
>   Aborted (core dumped)
>
> It's because the exclude_cmds() in libsubcmd has a use-after-free when
> it removes some entries.  After copying one to another entry, it keeps
> the pointer in the both position.  And the next copy operation will free
> the later one but it's the same entry in the previous one.
>
> For example, let's say cmds = { A, B, C, D, E } and excludes = { B, E }.
>
>   ci  cj  ei   cmds-name  excludes
>   -----------+--------------------
>    0   0   0 |     A         B       :    cmp < 0, ci == cj
>    1   1   0 |     B         B       :    cmp == 0
>    2   1   1 |     C         E       :    cmp < 0, ci != cj
>
> At this point, it frees cmds->names[1] and cmds->names[1] is assigned to
> cmds->names[2].
>
>    3   2   1 |     D         E       :    cmp < 0, ci != cj
>
> Now it frees cmds->names[2] but it's the same as cmds->names[1].  So
> accessing cmds->names[1] will be invalid.
>
> This makes the subcmd tests succeed.
>
>   $ perf test subcmd
>    69: libsubcmd help tests                                            :
>    69.1: Load subcmd names                                             : Ok
>    69.2: Uniquify subcmd names                                         : Ok
>    69.3: Exclude duplicate subcmd names                                : Ok
>
> Fixes: 657a3efee43a ("libsubcmd: Avoid SEGV/use-after-free when commands aren't excluded")
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Fwiw, it seems a shame we're doing this and the code in git is drifting apart:

https://github.com/git/git/blob/83014dc05f6fc9275c0a02886cb428805abaf9e5/help.c#L204

Thanks,
Ian




> ---
>  tools/lib/subcmd/help.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/subcmd/help.c b/tools/lib/subcmd/help.c
> index 8561b0f01a247690..9ef569492560efd7 100644
> --- a/tools/lib/subcmd/help.c
> +++ b/tools/lib/subcmd/help.c
> @@ -9,6 +9,7 @@
>  #include <sys/stat.h>
>  #include <unistd.h>
>  #include <dirent.h>
> +#include <assert.h>
>  #include "subcmd-util.h"
>  #include "help.h"
>  #include "exec-cmd.h"
> @@ -82,10 +83,11 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
>                                 ci++;
>                                 cj++;
>                         } else {
> -                               zfree(&cmds->names[cj]);
> -                               cmds->names[cj++] = cmds->names[ci++];
> +                               cmds->names[cj++] = cmds->names[ci];
> +                               cmds->names[ci++] = NULL;
>                         }
>                 } else if (cmp == 0) {
> +                       zfree(&cmds->names[ci]);
>                         ci++;
>                         ei++;
>                 } else if (cmp > 0) {
> @@ -94,12 +96,12 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
>         }
>         if (ci != cj) {
>                 while (ci < cmds->cnt) {
> -                       zfree(&cmds->names[cj]);
> -                       cmds->names[cj++] = cmds->names[ci++];
> +                       cmds->names[cj++] = cmds->names[ci];
> +                       cmds->names[ci++] = NULL;
>                 }
>         }
>         for (ci = cj; ci < cmds->cnt; ci++)
> -               zfree(&cmds->names[ci]);
> +               assert(cmds->names[ci] == NULL);
>         cmds->cnt = cj;
>  }
>
> --
> 2.50.0.727.gbf7dc18ff4-goog
>