Re: [PATCH 1/2] perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse()

[Ian Rogers]

On Wed, Aug 5, 2026 at 8:11 AM Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>
> hpp_list__parse() has three bugs:
>
> 1. The PARSE_LIST macro resets ret = 0 at the start of each invocation,
>    so an error from output parsing is silently overwritten when the sort
>    parsing block runs.  The function returns success with partially
>    initialized state.
>
> 2. When the caller passes a non-NULL output_ or sort_ string, but
>    strdup() returns NULL due to OOM, NULL is passed to PARSE_LIST which
>    treats it as empty input (the "if (!_list) break" branch).  No error
>    is returned.
>
> 3. When the called _fn function fails and returns something other than
>    -ESRCH or -EINVAL (-ENOMEM, for instance) it was not bailing out of
>    the strtok loop.
>
> Fix them by checking strdup() return values before proceeding and adding
> a cleanup label so that ret from each PARSE_LIST call is checked before
> the next runs, preserving the first error.
>
> The early exits now skip perf_hpp__setup_output_field(), which means
> c2c_hists__reinit() can return a non-zero value in cases that previously
> always succeeded silently.  Both callers discarded its return:
> resort_cl_cb() continued into hists__collapse_resort() on a broken list,
> and perf_c2c__report() proceeded with uninitialised hists.  Fix the full
> chain: check and propagate the error in resort_cl_cb() -- hists__iterate_cb()
> already stops iteration and returns the callback error -- and check both
> c2c_hists__reinit() and hists__iterate_cb() in perf_c2c__report().
>
> Also turn PARSE_LIST into a function, using a switch to catch other
> errors, converting the called functions to return an appropriate errno
> instead of -1 on failure.
>
> Also make the two callers that iterate sort_dimension__add() and
> output_field_add() handle the newly propagated errors: setup_sort_list()
> and setup_output_list() only checked for -EINVAL and -ESRCH, so an
> -ENOMEM from a failed allocation was silently overwritten by the next
> loop iteration.  Break out of the loop and propagate any other error.
>
> The hpp_list__parse() fixes were developed with AI assistance from
> Claude:claude-sonnet-4.6, and the setup_sort_list()/setup_output_list()
> caller fixes with AI assistance from Opencode:mimo-v2.5-free and
> Opencode:DeepSeek-V4-Flash-free.
>
> Fixes: 2d388bd0c9d3 ("perf c2c report: Add stdio output support")
> Reported-by: sashiko-bot <sashiko-bot@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Assisted-by: Claude:claude-sonnet-4.6
> Assisted-by: Opencode:mimo-v2.5-free
> Assisted-by: Opencode:DeepSeek-V4-Flash-free
> Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks,
Ian

> ---
>  tools/perf/builtin-c2c.c | 85 ++++++++++++++++++++++++++++------------
>  tools/perf/util/sort.c   | 76 +++++++++++++++++++++++------------
>  2 files changed, 112 insertions(+), 49 deletions(-)
>
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77afe8..160b82694d391c50 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -12,11 +12,14 @@
>   */
>  #include <errno.h>
>  #include <inttypes.h>
> +#include <stdlib.h>
> +#include <string.h>
>
>  #include <asm/bug.h>
>  #include <linux/compiler.h>
>  #include <linux/err.h>
>  #include <linux/kernel.h>
> +#include <linux/string.h>
>  #include <linux/stringify.h>
>  #include <linux/zalloc.h>
>  #include <sys/param.h>
> @@ -2063,26 +2066,38 @@ static int c2c_hists__init_sort(struct perf_hpp_list *hpp_list, char *name, stru
>         return 0;
>  }
>
> -#define PARSE_LIST(_list, _fn)                                                 \
> -       do {                                                                    \
> -               char *tmp, *tok;                                                \
> -               ret = 0;                                                        \
> -                                                                               \
> -               if (!_list)                                                     \
> -                       break;                                                  \
> -                                                                               \
> -               for (tok = strtok_r((char *)_list, ", ", &tmp);                 \
> -                               tok; tok = strtok_r(NULL, ", ", &tmp)) {        \
> -                       ret = _fn(hpp_list, tok, env);                          \
> -                       if (ret == -EINVAL) {                                   \
> -                               pr_err("Invalid --fields key: `%s'", tok);      \
> -                               break;                                          \
> -                       } else if (ret == -ESRCH) {                             \
> -                               pr_err("Unknown --fields key: `%s'", tok);      \
> -                               break;                                          \
> -                       }                                                       \
> -               }                                                               \
> -       } while (0)
> +static int __hpp_list__parse(struct perf_hpp_list *hpp_list, char *_list, struct perf_env *env,
> +                            int (*_fn)(struct perf_hpp_list *hpp_list, char *name, struct perf_env *env))
> +{
> +       char *tmp, *tok;
> +       int ret = 0;
> +
> +       if (!_list)
> +               return 0;
> +
> +       for (tok = strtok_r(_list, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
> +               ret = _fn(hpp_list, tok, env);
> +               switch (ret) {
> +               case 0:
> +                       continue;
> +               case -EINVAL:
> +                       pr_err("Invalid --fields key: `%s'", tok);
> +                       goto out;
> +               case -ESRCH:
> +                       pr_err("Unknown --fields key: `%s'", tok);
> +                       goto out;
> +               default: {
> +                       char buf[STRERR_BUFSIZE];
> +
> +                       pr_err("%s for --fields key: `%s'",
> +                              str_error_r(-ret, buf, sizeof(buf)), tok);
> +                       goto out;
> +               }
> +               }
> +       }
> +out:
> +       return ret;
> +}
>
>  static int hpp_list__parse(struct perf_hpp_list *hpp_list,
>                            const char *output_,
> @@ -2093,8 +2108,18 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
>         char *sort   = sort_   ? strdup(sort_) : NULL;
>         int ret;
>
> -       PARSE_LIST(output, c2c_hists__init_output);
> -       PARSE_LIST(sort,   c2c_hists__init_sort);
> +       /* strdup() returns NULL on OOM, don't silently treat as empty */
> +       if ((output_ && !output) || (sort_ && !sort)) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       ret = __hpp_list__parse(hpp_list, output, env, c2c_hists__init_output);
> +       if (ret)
> +               goto out;
> +       ret = __hpp_list__parse(hpp_list, sort, env, c2c_hists__init_sort);
> +       if (ret)
> +               goto out;
>
>         /* copy sort keys to output fields */
>         perf_hpp__setup_output_field(hpp_list);
> @@ -2111,6 +2136,7 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
>         perf_hpp__append_sort_keys(&hists->list);
>  #endif
>
> +out:
>         free(output);
>         free(sort);
>         return ret;
> @@ -2281,6 +2307,7 @@ static int resort_cl_cb(struct hist_entry *he, void *arg)
>         struct c2c_hist_entry *c2c_he;
>         struct c2c_hists *c2c_hists;
>         bool display = he__display(he, &c2c.shared_clines_stats);
> +       int ret;
>
>         c2c_he = container_of(he, struct c2c_hist_entry, he);
>         c2c_hists = c2c_he->hists;
> @@ -2291,7 +2318,9 @@ static int resort_cl_cb(struct hist_entry *he, void *arg)
>                 c2c_he->cacheline_idx = idx++;
>                 calc_width(c2c_he);
>
> -               c2c_hists__reinit(c2c_hists, c2c.cl_output, c2c.cl_resort, env);
> +               ret = c2c_hists__reinit(c2c_hists, c2c.cl_output, c2c.cl_resort, env);
> +               if (ret)
> +                       return ret;
>
>                 hists__collapse_resort(&c2c_hists->hists, NULL);
>                 hists__output_resort_cb(&c2c_hists->hists, NULL, filter_cb);
> @@ -3356,13 +3385,19 @@ static int perf_c2c__report(int argc, const char **argv)
>         else if (c2c.display == DISPLAY_SNP_PEER)
>                 sort_str = "tot_peer";
>
> -       c2c_hists__reinit(&c2c.hists, output_str, sort_str, perf_session__env(session));
> +       err = c2c_hists__reinit(&c2c.hists, output_str, sort_str, perf_session__env(session));
> +       if (err) {
> +               pr_err("Failed to reinitialize hists\n");
> +               goto out_mem2node;
> +       }
>
>         ui_progress__init(&prog, c2c.hists.hists.nr_entries, "Sorting...");
>
>         hists__collapse_resort(&c2c.hists.hists, NULL);
>         hists__output_resort_cb(&c2c.hists.hists, &prog, resort_shared_cl_cb);
> -       hists__iterate_cb(&c2c.hists.hists, resort_cl_cb, perf_session__env(session));
> +       err = hists__iterate_cb(&c2c.hists.hists, resort_cl_cb, perf_session__env(session));
> +       if (err)
> +               goto out_mem2node;
>
>         ui_progress__finish();
>
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index dcf9189786f8aeba..58638ec9ae0ede7f 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -3105,7 +3105,7 @@ static int __sort_dimension__add_hpp_sort(struct sort_dimension *sd,
>         struct hpp_sort_entry *hse = __sort_dimension__alloc_hpp(sd, level);
>
>         if (hse == NULL)
> -               return -1;
> +               return -ENOMEM;
>
>         perf_hpp_list__register_sort_field(list, &hse->hpp);
>         return 0;
> @@ -3118,7 +3118,7 @@ static int __sort_dimension__add_hpp_output(struct sort_dimension *sd,
>         struct hpp_sort_entry *hse = __sort_dimension__alloc_hpp(sd, level);
>
>         if (hse == NULL)
> -               return -1;
> +               return -ENOMEM;
>
>         perf_hpp_list__column_register(list, &hse->hpp);
>         return 0;
> @@ -3742,14 +3742,18 @@ static int __sort_dimension__add(struct sort_dimension *sd,
>                                  struct perf_hpp_list *list,
>                                  int level)
>  {
> +       int ret;
> +
>         if (sd->taken)
>                 return 0;
>
> -       if (__sort_dimension__add_hpp_sort(sd, list, level) < 0)
> -               return -1;
> +       ret = __sort_dimension__add_hpp_sort(sd, list, level);
> +       if (ret < 0)
> +               return ret;
>
> -       if (__sort_dimension__update(sd, list) < 0)
> -               return -1;
> +       ret = __sort_dimension__update(sd, list);
> +       if (ret < 0)
> +               return ret;
>
>         sd->taken = 1;
>
> @@ -3767,7 +3771,7 @@ static int __hpp_dimension__add(struct hpp_dimension *hd,
>
>         fmt = __hpp_dimension__alloc_hpp(hd, level);
>         if (!fmt)
> -               return -1;
> +               return -ENOMEM;
>
>         hd->taken = 1;
>         hd->was_taken = 1;
> @@ -3779,14 +3783,18 @@ static int __sort_dimension__add_output(struct perf_hpp_list *list,
>                                         struct sort_dimension *sd,
>                                         int level)
>  {
> +       int ret;
> +
>         if (sd->taken)
>                 return 0;
>
> -       if (__sort_dimension__add_hpp_output(sd, list, level) < 0)
> -               return -1;
> +       ret = __sort_dimension__add_hpp_output(sd, list, level);
> +       if (ret < 0)
> +               return ret;
>
> -       if (__sort_dimension__update(sd, list) < 0)
> -               return -1;
> +       ret = __sort_dimension__update(sd, list);
> +       if (ret < 0)
> +               return ret;
>
>         sd->taken = 1;
>         return 0;
> @@ -3803,7 +3811,7 @@ static int __hpp_dimension__add_output(struct perf_hpp_list *list,
>
>         fmt = __hpp_dimension__alloc_hpp(hd, level);
>         if (!fmt)
> -               return -1;
> +               return -ENOMEM;
>
>         hd->taken = 1;
>         perf_hpp_list__column_register(list, fmt);
> @@ -3869,8 +3877,7 @@ int sort_dimension__add(struct perf_hpp_list *list, const char *tok,
>                                     strlen(tok)))
>                         return -EINVAL;
>
> -               __sort_dimension__add(sd, list, level);
> -               return 0;
> +               return __sort_dimension__add(sd, list, level);
>         }
>
>         for (i = 0; i < ARRAY_SIZE(memory_sort_dimensions); i++) {
> @@ -3882,8 +3889,7 @@ int sort_dimension__add(struct perf_hpp_list *list, const char *tok,
>                 if (sort__mode != SORT_MODE__MEMORY)
>                         return -EINVAL;
>
> -               __sort_dimension__add(sd, list, level);
> -               return 0;
> +               return __sort_dimension__add(sd, list, level);
>         }
>
>         for (i = 0; i < ARRAY_SIZE(hpp_sort_dimensions); i++) {
> @@ -3973,15 +3979,25 @@ static int setup_sort_list(struct perf_hpp_list *list, char *str,
>                         }
>
>                         ret = sort_dimension__add(list, tok, evlist, env, level);
> -                       if (ret == -EINVAL) {
> +                       switch (ret) {
> +                       case 0:
> +                               break;
> +                       case -EINVAL:
>                                 if (!cacheline_size() && !strncasecmp(tok, "dcacheline", strlen(tok)))
>                                         ui__error("The \"dcacheline\" --sort key needs to know the cacheline size and it couldn't be determined on this system");
>                                 else
>                                         ui__error("Invalid --sort key: `%s'", tok);
> -                               break;
> -                       } else if (ret == -ESRCH) {
> +                               goto out;
> +                       case -ESRCH:
>                                 ui__error("Unknown --sort key: `%s'", tok);
> -                               break;
> +                               goto out;
> +                       default: {
> +                               char buf[STRERR_BUFSIZE];
> +
> +                               ui__error("%s for --sort key: `%s'",
> +                                         str_error_r(-ret, buf, sizeof(buf)), tok);
> +                               goto out;
> +                       }
>                         }
>                         prev_level = level;
>                 }
> @@ -3989,6 +4005,7 @@ static int setup_sort_list(struct perf_hpp_list *list, char *str,
>                 level = next_level;
>         } while (tmp);
>
> +out:
>         return ret;
>  }
>
> @@ -4315,15 +4332,26 @@ static int setup_output_list(struct perf_hpp_list *list, char *str)
>         for (tok = strtok_r(str, ", ", &tmp);
>                         tok; tok = strtok_r(NULL, ", ", &tmp)) {
>                 ret = output_field_add(list, tok, &level);
> -               if (ret == -EINVAL) {
> -                       ui__error("Invalid --fields key: `%s'", tok);
> +               switch (ret) {
> +               case 0:
>                         break;
> -               } else if (ret == -ESRCH) {
> +               case -EINVAL:
> +                       ui__error("Invalid --fields key: `%s'", tok);
> +                       goto out;
> +               case -ESRCH:
>                         ui__error("Unknown --fields key: `%s'", tok);
> -                       break;
> +                       goto out;
> +               default: {
> +                       char buf[STRERR_BUFSIZE];
> +
> +                       ui__error("%s for --fields key: `%s'",
> +                                 str_error_r(-ret, buf, sizeof(buf)), tok);
> +                       goto out;
> +               }
>                 }
>         }
>
> +out:
>         return ret;
>  }
>
> --
> 2.55.0
>