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

[Namhyung Kim <namhyung@kernel.org>]

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.

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