Re: [PATCH 3/9] perf thread-stack: Bound wrapped branch stack copy

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:

Pre-existing issues:
- [Medium] Unconditional dereferences of `ts->br_stack_rb` cause a segmenta=
tion fault if thread stack initialization fails or if it was initially crea=
ted without a branch stack.
--

--- Patch [3]: [PATCH 3/9] perf thread-stack: Bound wrapped branch stack co=
py ---
commit 3efa75c3f04529593a5a6a18971bae8fa6a9617c
Author: Amir Ayupov <[email protected]>

perf thread-stack: Bound wrapped branch stack copy

This commit fixes a potential destination buffer overrun when copying
wrapped branch stack entries. It correctly limits the number of entries
copied to the remaining destination size.

> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stac=
k.c
> index 1a3dffa83bde2..51eaedb47bb1d 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -643,7 +643,7 @@ void thread_stack__br_sample(struct thread *thread, i=
nt cpu,

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference earlier in this function.

Could src be NULL when we evaluate src->nr?

tools/perf/util/thread-stack.c:thread_stack__br_sample() {
        ...
        src =3D ts->br_stack_rb;
        if (!src->nr)
                return;
        ...
}

It looks like this might occur if thread_stack__init() fails to allocate the
branch stack ring buffer, as the partially initialized thread_stack is
already attached to the thread:

tools/perf/util/thread-stack.c:thread_stack__new() {
        ...
        thread__set_ts(thread, new_ts);
        ts =3D new_ts;
        ...
        if (!ts->stack &&
            thread_stack__init(ts, thread, crp, callstack, br_stack_sz))
                return NULL;
        ...
}

If a caller ignores the error from thread_stack__event(), they might proceed
to call thread_stack__br_sample() or thread_stack__update_br_stack() with a
NULL branch stack ring buffer. Is there a need to check if ts->br_stack_rb
is populated before dereferencing it here?

>  		sz -=3D nr;
>  		be =3D &dst->entries[nr];
>  		nr =3D min(ts->br_stack_pos, sz);
> -		memcpy(be, &src->entries[0], bsz * ts->br_stack_pos);
> +		memcpy(be, &src->entries[0], bsz * nr);
>  	}
>  }

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803090640.2412=
[email protected]?part=3D3