Re: [PATCH 08/13] perf sched: Replace (void*)1 sentinel with proper runtime allocation

[Ian Rogers]

On Fri, Jun 12, 2026 at 3:24 PM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>
> map__findnew_thread() marks color-pid threads by storing (void*)1 as
> the thread private data via thread__set_priv().  This sentinel value
> causes two problems:
>
> 1. thread__get_runtime() returns (void*)1 as a struct thread_runtime
>    pointer.  Any field access (e.g. tr->shortname) dereferences address
>    1, which is an unmapped page — immediate segfault.
>
> 2. cmd_sched() registers free() as the thread priv destructor, so thread
>    cleanup calls free((void*)1) — undefined behavior that corrupts the
>    heap on many allocators.
>
> Fix by adding a 'color' flag to struct thread_runtime and allocating a
> real runtime struct for color-pid threads.  thread__has_color() now
> checks the flag instead of relying on priv being non-NULL.
>
> Reported-by: sashiko-bot <sashiko-bot@xxxxxxxxxx>
> Fixes: 58a606149c60d5da ("perf sched: Avoid union type punning undefined behavior")
> Cc: Ian Rogers <irogers@xxxxxxxxxx>
> Assisted-by: Claude Opus 4.6 <noreply@xxxxxxxxxxxxx>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks,
Ian

> ---
>  tools/perf/builtin-sched.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 5f16caebd9645da0..7fd63a9db4574fbf 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -274,6 +274,7 @@ struct thread_runtime {
>         u64 migrations;
>
>         int prio;
> +       bool color;
>  };
>
>  /* per event run time data */
> @@ -1589,22 +1590,32 @@ static int process_sched_wakeup_ignore(const struct perf_tool *tool __maybe_unus
>
>  static bool thread__has_color(struct thread *thread)
>  {
> -       return thread__priv(thread) != NULL;
> +       struct thread_runtime *tr = thread__priv(thread);
> +
> +       return tr != NULL && tr->color;
>  }
>
>  static struct thread*
>  map__findnew_thread(struct perf_sched *sched, struct machine *machine, pid_t pid, pid_t tid)
>  {
>         struct thread *thread = machine__findnew_thread(machine, pid, tid);
> -       bool color = false;
>
> -       if (!sched->map.color_pids || !thread || thread__priv(thread))
> +       if (!sched->map.color_pids || !thread)
>                 return thread;
>
> -       if (thread_map__has(sched->map.color_pids, tid))
> -               color = true;
> +       /*
> +        * Always check the color-pids map, even if thread__priv() is
> +        * already set.  COMM events processed before the first sched_switch
> +        * allocate a thread_runtime via thread__get_runtime(), so priv is
> +        * non-NULL before we ever get here.  Skipping the check on non-NULL
> +        * priv would prevent those threads from being colored.
> +        */
> +       if (thread_map__has(sched->map.color_pids, tid)) {
> +               struct thread_runtime *tr = thread__get_runtime(thread);
>
> -       thread__set_priv(thread, color ? ((void*)1) : NULL);
> +               if (tr)
> +                       tr->color = true;
> +       }
>         return thread;
>  }
>
> --
> 2.54.0
>