On Friday, 3 July 2026 14:21:29 CEST Peter Maydell wrote:
> On Fri, 3 Jul 2026 at 12:41, Christian Schoenebeck
>
> <[email protected]> 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
