Re: [PATCH 13/23] perf tools: Fix uninitialized pathname on uncompressed fallback in filename__decompress()

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

On Wed, Jun 10, 2026 at 08:08:10PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] The commit message explicitly describes a logic flow that would cause a severe data-loss bug, while the code implements the opposite (safe) logic.
> 
> Pre-existing issues:
> - [High] Missing error code propagation and improper negative error handling cause `__open_dso` to mistakenly return valid file descriptors on failure.
> --
> 
> commit 5ae85aedfdb730a58a0d285fd77526ec0c92c20e
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> perf tools: Fix uninitialized pathname on uncompressed fallback in filename__decompress()
> 
> This commit addresses an issue where filename__decompress() left the
> pathname parameter uninitialized on the early return path for uncompressed
> files. The code is updated to set an empty string in this case, and
> callers are modified to check for this empty string condition.
> 
> > Fix by copying the original filename to pathname before the early return,
> > matching the behavior of the normal decompression path.
> 
> [Severity: Medium]
> Does this commit message accurately describe the implementation?
> 
> The code below sets pathname[0] = '\0' instead of copying the filename. If
> the code had copied the original filename as described here, wouldn't callers
> assume the populated pathname was a newly created temporary file and call
> unlink() on it, deleting the user's uncompressed kernel module or binary?

So this was the only non pre-existing issue found in this series, I
rebased it on top of the 28 patches I merged in perf-tools-next while
waiting for this last series to be reviewed and will do a full container
suite test build, then push everything publicly.

- Arnaldo
 
> [ ... ]
> 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index ee06a252a54d3..6a34717c9f31f 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -343,8 +343,11 @@ int filename__decompress(const char *name, char *pathname,
> >  	 * To keep this transparent, we detect this and return the file
> >  	 * descriptor to the uncompressed file.
> >  	 */
> > -	if (!compressions[comp].is_compressed(name))
> > +	if (!compressions[comp].is_compressed(name)) {
> > +		if (pathname && len > 0)
> > +			pathname[0] = '\0';
> >  		return open(name, O_RDONLY | O_CLOEXEC);
> 
> [Severity: High]
> This is a pre-existing issue, but if open() fails here, it returns -1
> without populating the *err pointer.
> 
> If open() fails, could this sequence occur?
> 
> filename__decompress()
>    returns -1 without setting *err
> 
> dso__get_filename() catches the failure:
>    errno = *dso__load_errno(dso); // errno is overwritten with 0
> 
> __open_dso() handles the NULL return:
>    fd = -errno; // results in fd = 0
> 
> Alternatively, if decompression fails later in this function, *err is
> populated with DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE (which expands to
> -10005).
> 
> In that case, would errno become -10005, and fd = -(-10005) = 10005?
> 
> Since both 0 and 10005 are non-negative, does this cause callers to treat
> the operation as successful and attempt to read from standard input or a
> bogus file descriptor?
> 
> > +	}
> >  
> >  	fd = mkostemp(tmpbuf, O_CLOEXEC);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260610195157.2091137-1-acme@kernel.org?part=13