Re: [PULL 3/9] 9pfs: local : Introduce local_fid_fd() helper

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Friday, 3 July 2026 12:52:24 CEST Peter Maydell wrote:
> On Mon, 5 May 2025 at 10:54, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > From: Greg Kurz <groug@kaod.org>
> > 
> > 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_oss@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