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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Friday, 3 July 2026 14:21:29 CEST Peter Maydell wrote:
> On Fri, 3 Jul 2026 at 12:41, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > On Friday, 3 July 2026 12:52:24 CEST Peter Maydell wrote:
[...]
> > 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.
> 
> That part is fine. What Coverity is warning about is the callsites:

I know, this is just the change that triggered Coverity's warning, because it 
made Coverity think that this function might return -1, even though this 
branch is actually *never* executed.

> > > > +
> > > > 
> > > >  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.
> 
> In practice, yes, but it's not a valid file descriptor. Coverity
> warns about it because it tends to mean "missing error handling",
> as here.

Yes, -1 is defined by POSIX as "invalid file descriptor". And that's the point 
why the helper function would return it as last-resort result, because -1 is 
well defined by POSIX to not be interchanged with a real file descriptor.

To make this clear: before this change, this helper could potentially return 
garbage.

And yes, you can check for every -1 file descriptor just before passing it to 
a syscall, even though I already explained that a) you never ever get there 
because this check is already made at the higher level call stack, and b) that 
every syscall handles -1 as file descriptor gracefully.

If you still think this was not a false positive, you are welcome to send a 
patch.

Thank you!

/Christian