Re: [PATCH 05/12] perf jitdump: Check snprintf return before computing header size

On Wed, Aug 05, 2026 at 02:29:47PM -0700, Ian Rogers wrote:
> On Wed, Aug 5, 2026 at 12:45 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > On Wed, Aug 05, 2026 at 12:07:23PM -0700, Ian Rogers wrote:
> > > On Wed, Aug 5, 2026 at 6:31 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > > +++ b/tools/perf/util/jitdump.c
> > > > @@ -493,6 +493,9 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
> > > >                         jd->dir,
> > > >                         nspid,
> > > >                         count);
> > > > +       /* snprintf returns would-be length on truncation, clamp to buffer */
> > > > +       if (size >= PATH_MAX)
> > > > +               size = PATH_MAX - 1;

> > > Given the recent fixes to reading /proc/pid/maps where it was assumed
> > > the file paths would be limited to PATH_MAX and it turns out that
> > > PATH_MAX doesn't really do that and we had potential buffer overruns
> > > during synthesis, I wonder it would be more intention revealing here
> > > to use "sizeof(event->mmap2.filename)" rather than PATH_MAX. Other
> > > than the constant used, I agree with the change and using PATH_MAX
> > > isn't wrong.

> > We need to go on having our tools/perf/AGENTS.md with all those rules
> > :-)
 
> Yeah, I don't know of a plan for AGENTS.md files but there is always:
> https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/perf.md
 
> Perhaps we can add something like:
> ```
> ## PATH_MAX avoidance
> 
> The PATH_MAX constant isn't a limit to paths in places like
> /proc/pid/maps, and reading paths from these locations can lead to
> buffer overruns for buffers sized at PATH_MAX - as such prefer dynamic
> memory allocation for paths. When referring to the size of PATH_MAX
> sized char arrays in kernel system call and ring buffer data
> structures, prefer to use sizeof the variable rather than the PATH_MAX
> constant to make it clear the size and possible truncation is being
> enforced by the kernel which may substitute long paths with the value
> "//toolong".
> ```

Looks sensible, I encourage you to submit a patch adding that:

Reviewed-by: Arnaldo Carvalho de Melo <[email protected]>

- Arnaldo