On Friday, 3 July 2026 12:52:24 CEST Peter Maydell wrote:
> On Mon, 5 May 2025 at 10:54, Christian Schoenebeck
>
> <[email protected]> wrote:
> > From: Greg Kurz <[email protected]>
> >
> > Factor out duplicated code to a single helper. More users to come.
>
> Hi; Coverity complains about the callsites of this new
>
> helper (CID 1660923, 1660924, 1660925):
Hi Peter,
I have seen these new CID reports. CID 1660926 (test code only) is a valid one
where I already have a small patch prepared to be sent in the next few days.
However CID 1660923, 1660924, 1660925 are false positives AFAICS:
> > +static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> > +{
> > + if (fid_type == P9_FID_DIR) {
> > + return dirfd(fs->dir.stream);
> > + } else {
> > + return fs->fd;
> > + }
> > +}
>
> dirfd() can fail and return -1, so this function is "returns a
> file descriptor, or -1 on error"...
That's the *old* helper function. Apparently you were replying on the wrong PR
series (from 2025!).
CID 1660923, 1660924, 1660925 were triggered by this change
(9p queue 2026-06-29, [PULL 23/23] hw/9pfs/local: harden local_fid_fd() on FID
types) [1]:
[1]
https://lore.kernel.org/qemu-devel/75893c058b21d87d1ec66bbd4e8bf84e1fd616d1.1782739719.git.qemu_...@crudebyte.com/
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 4708e170a4..ee592b62f8 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -775,8 +775,11 @@ static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
if (fid_type == P9_FID_DIR) {
return dirfd(fs->dir.stream);
- } else {
+ } else if (fid_type == P9_FID_FILE) {
return fs->fd;
+ } else {
+ errno = EBADF;
+ return -1;
}
}
However, that is just a last resort safety net! In 9p.c we are already
checking via fid_has_valid_file_handle(), and in the local fs backend via
local_has_valid_file_handle(), and in both cases would leave via error path. I
have not identified any call path where we would not guard this, so we should
never end up passing -1 as FD to a syscall.
So this new EBADF / -1 path in local_fid_fd() is just to avoid undefined
behaviour as a last resort measure, in case a guard was forgotten in future
changes.
> > +
> >
> > static int local_fstat(FsContext *fs_ctx, int fid_type,
> >
> > V9fsFidOpenState *fs, struct stat *stbuf)
> >
> > {
> >
> > - int err, fd;
> > -
> > - if (fid_type == P9_FID_DIR) {
> > - fd = dirfd(fs->dir.stream);
> > - } else {
> > - fd = fs->fd;
> > - }
> > + int err, fd = local_fid_fd(fid_type, fs);
> >
> > err = fstat(fd, stbuf);
>
> ...but the callsites don't check for error, and instead can
> feed -1 into fstat() or other functions, which isn't a valid
> filedescriptor.
But even then, a syscall receiving -1 for a FD argument would handle this
gracefully by returning -1 and errno EBADF.
/Christian
