Re: [PATCH] perf jitdump: Fix a build error with ASAN

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Fri, Jun 05, 2026 at 04:22:20PM -0700, Namhyung Kim wrote:
> On Fri, Jun 05, 2026 at 07:03:49AM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
> > 
> > Pre-existing issues:
> > - [High] Unlocking an unlocked stream on the error path causes undefined behavior.
> > - [High] Incorrect logic for skipping extended jitdump headers leads to parsing failure.
> > - [High] Ignoring the return value of `dirname()` breaks directory extraction and causes ENOTDIR failures.
> > - [High] If the generated JIT ELF path exceeds PATH_MAX, snprintf truncation causes size underflow resulting in a heap buffer overflow.
> > - [High] Cross-endian byte swapping of JIT_CODE_DEBUG_INFO records corrupts data by ignoring the flexible array member.
> > - [High] Lack of validation on `code_size` enables arbitrary out-of-bounds read and memory leakage.
> > - [Medium] Memory leak of `debug_data` and `unwinding_data` pointers.
> > - [Medium] Memory leak of `event` in `jit_repipe_code_move`.
> > - [Medium] Ineffective inter-process synchronization using `flockfile`.
> > - [Medium] Integer underflow leading to massive malloc in `jit_repipe_debug_info`.
> 
> I don't plan to touch any of these issues, at least in the near future.

LOL, Sashiko... I'll ask Claude to add these in my TODOs,

Thanks, applied to perf-tools-next, for v7.2.

- Arnaldo
 
> Thanks,
> Namhyung
> 
> > --
> > 
> > commit f314399f4edf9366c3ee4d529f45cb2d7e7d454d
> > Author: Namhyung Kim <namhyung@kernel.org>
> > 
> > perf jitdump: Fix a build error with ASAN
> > 
> > This patch resolves a stringop-truncation build error when compiling perf
> > with ASAN on GCC 16. It reduces the strncpy bound by one byte to avoid
> > truncation warnings.
> > 
> > > diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> > > index e0ce8b9047298..16390e7f9062d 100644
> > > --- a/tools/perf/util/jitdump.c
> > > +++ b/tools/perf/util/jitdump.c
> > > @@ -234,7 +234,7 @@ jit_open(struct jit_buf_desc *jd, const char *name)
> > >  	/*
> > >  	 * keep dirname for generating files and mmap records
> > >  	 */
> > > -	strncpy(jd->dir, name, PATH_MAX);
> > > +	strncpy(jd->dir, name, PATH_MAX - 1);
> > >  	jd->dir[PATH_MAX - 1] = '\0';
> > >  	dirname(jd->dir);
> > 
> > [Severity: High]
> > This isn't a bug introduced by this patch, but is it correct to ignore the
> > return value of dirname()?
> > 
> > If jd->dir contains no slashes, glibc dirname() returns a pointer to "." but
> > leaves the original string unmodified. Since the return value is discarded,
> > jd->dir retains the full filename instead of becoming ".". This could cause
> > later snprintf calls to use the target file as a directory component, leading
> > to ENOTDIR failures.
> > 
> > >  	free(buf);
> > 
> > [Severity: High]
> > This is a pre-existing issue, but does the error handling path incorrectly
> > unlock an unlocked stream?
> > 
> > If malloc() fails early in jit_open(), it jumps to the error label:
> > 
> > tools/perf/util/jitdump.c:jit_open() {
> >     ...
> > 	buf = malloc(bsz);
> > 	if (!buf)
> > 		goto error;
> > 
> > 	/*
> > 	 * protect from writer modifying the file while we are reading it
> > 	 */
> > 	flockfile(jd->in);
> >     ...
> > error:
> > 	free(buf);
> > 	funlockfile(jd->in);
> > 	fclose(jd->in);
> > 	return retval;
> > }
> > 
> > Since flockfile() is only called after malloc() succeeds, calling funlockfile()
> > here results in undefined behavior on an unlocked stream.
> > 
> > [Severity: High]
> > This is a pre-existing issue, but does this logic break extended header reads?
> > 
> > In jit_open(), when handling extended headers, bsz is updated to bs right
> > before calculating the read size:
> > 
> > tools/perf/util/jitdump.c:jit_open() {
> >     ...
> > 	if (bs > bsz) {
> > 		n = realloc(buf, bs);
> > 		if (!n)
> > 			goto error;
> > 		bsz = bs;
> > 		buf = n;
> > 		/* read extra we do not know about */
> > 		ret = fread(buf, bs - bsz, 1, jd->in);
> > 		if (ret != 1)
> > 			goto error;
> > 	}
> >     ...
> > }
> > 
> > Because bsz = bs, the read size bs - bsz is always 0. fread() will return 0,
> > causing the ret != 1 check to fail and unconditionally jump to the error path.
> > 
> > [Severity: High]
> > This is a pre-existing issue, but could snprintf() truncation cause an
> > out-of-bounds pointer calculation?
> > 
> > In jit_repipe_code_load(), snprintf() calculates a path:
> > 
> > tools/perf/util/jitdump.c:jit_repipe_code_load() {
> >     ...
> > 	filename = event->mmap2.filename;
> > 	size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so",
> > 			jd->dir,
> > 			nspid,
> > 			count);
> > 
> > 	size++; /* for \0 */
> > 
> > 	size = PERF_ALIGN(size, sizeof(u64));
> >     ...
> > 	event->mmap2.header.size = (sizeof(event->mmap2) -
> > 			(sizeof(event->mmap2.filename) - size) + idr_size);
> >     ...
> > 	id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
> > 	if (jd->sample_type & PERF_SAMPLE_TID) {
> > 		id->pid  = pid;
> >     ...
> > }
> > 
> > If the string exceeds PATH_MAX, snprintf() returns the untruncated length. The
> > expression sizeof(event->mmap2.filename) - size will then underflow because size
> > is greater than PATH_MAX, resulting in a massively inflated header.size. The
> > pointer id will point outside the allocated bounds, leading to heap corruption
> > when id->pid is assigned.
> > 
> > [Severity: High]
> > This isn't a bug introduced by this patch, but does the byte-swapping logic
> > corrupt data by ignoring a flexible array member?
> > 
> > In jit_get_next_entry():
> > 
> > tools/perf/util/jitdump.c:jit_get_next_entry() {
> >     ...
> > 	case JIT_CODE_DEBUG_INFO:
> > 		if (jd->needs_bswap) {
> > 			uint64_t n;
> > 			jr->info.code_addr = bswap_64(jr->info.code_addr);
> > 			jr->info.nr_entry  = bswap_64(jr->info.nr_entry);
> > 			for (n = 0 ; n < jr->info.nr_entry; n++) {
> > 				jr->info.entries[n].addr    = bswap_64(jr->info.entries[n].addr);
> > 				jr->info.entries[n].lineno  = bswap_32(jr->info.entries[n].lineno);
> > 				jr->info.entries[n].discrim = bswap_32(jr->info.entries[n].discrim);
> > 			}
> > 		}
> > 		break;
> >     ...
> > }
> > 
> > struct debug_entry contains a flexible array member (const char name[]). Using
> > array indexing (jr->info.entries[n]) assumes fixed-size elements. This will
> > calculate incorrect offsets that land inside the variable-length strings of
> > earlier entries, overwriting them with byte-swapped data.
> > 
> > [Severity: Medium]
> > This is a pre-existing issue, but do jd->debug_data and jd->unwinding_data leak?
> > 
> > In jit_repipe_debug_info(), memory is allocated and assigned:
> > 
> > tools/perf/util/jitdump.c:jit_repipe_debug_info() {
> >     ...
> > 	data = malloc(sz);
> > 	if (!data)
> > 		return -1;
> > 
> > 	memcpy(data, &jr->info.entries, sz);
> > 
> > 	jd->debug_data       = data;
> >     ...
> > }
> > 
> > While these are freed during jit_repipe_code_load(), if a JIT dump terminates
> > early, lacks trailing load records, or contains consecutive debug records, the
> > previously allocated buffers are overwritten or never freed. Neither jit_close()
> > nor jit_process() cleans up jd->debug_data or jd->unwinding_data.
> > 
> > [Severity: Medium]
> > This isn't a bug introduced by this patch, but does jit_repipe_code_move() leak
> > its event allocation?
> > 
> > tools/perf/util/jitdump.c:jit_repipe_code_move() {
> >     ...
> > 	event = calloc(1, sizeof(*event) + 16);
> > 	if (!event)
> > 		return -1;
> >     ...
> > out:
> > 	perf_sample__exit(&sample);
> > 	return ret;
> > }
> > 
> > The event buffer is dynamically allocated but there is no free(event) in the
> > cleanup block, leaking the memory for every JIT_CODE_MOVE record processed.
> > 
> > [Severity: Medium]
> > This is a pre-existing issue, but is flockfile() being used incorrectly for
> > inter-process synchronization?
> > 
> > In jit_open():
> > 
> > tools/perf/util/jitdump.c:jit_open() {
> >     ...
> > 	/*
> > 	 * protect from writer modifying the file while we are reading it
> > 	 */
> > 	flockfile(jd->in);
> >     ...
> > }
> > 
> > flockfile() is a POSIX thread mutex that provides no OS-level file locking.
> > It cannot prevent an external process from modifying the file concurrently,
> > making this synchronization completely ineffective against external writers.
> > 
> > [Severity: Medium]
> > This isn't a bug introduced by this patch, but could this allocation size calculation
> > underflow?
> > 
> > In jit_repipe_debug_info():
> > 
> > tools/perf/util/jitdump.c:jit_repipe_debug_info() {
> >     ...
> > 	sz  = jr->prefix.total_size - sizeof(jr->info);
> > 	data = malloc(sz);
> >     ...
> > }
> > 
> > If total_size is smaller than sizeof(jr->info), the unsigned subtraction will
> > underflow to a massive value, leading to a guaranteed malloc() failure or
> > instability if memory is overcommitted.
> > 
> > [Severity: High]
> > This is a pre-existing issue, but does lack of validation on code_size allow
> > out-of-bounds reads?
> > 
> > In jit_repipe_code_load():
> > 
> > tools/perf/util/jitdump.c:jit_repipe_code_load() {
> >     ...
> > 	csize = jr->load.code_size;
> >     ...
> > 	code  = (unsigned long)jr + jr->load.p.total_size - csize;
> >     ...
> > 	ret = jit_emit_elf(jd, filename, sym, addr, (const void *)uaddr, csize, ...
> >     ...
> > }
> > 
> > If an artificially large csize is parsed from the file without being bounds
> > checked against total_size, the code pointer calculation will underflow and
> > point into earlier heap memory. jit_emit_elf() will then dump csize bytes
> > from this out-of-bounds memory into the ELF file.
> > 
> > -- 
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260605064932.34316-1-namhyung@kernel.org?part=1