On Fri, 3 Jul 2026 at 13:49, Christian Schoenebeck
<[email protected]> wrote:
>
> 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:
> > > 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
We don't need to check before every syscall. We just need to
check the error return from functions that can fail after we
call them. This function can fail (because dirfd() can fail), so
we should explicitly check that.
>, even though I already explained that a) you never ever get there
> because this check is already made at the higher level call stack
If fid_type cannot possibly be anything other than one of the
two valid values (i.e. anything else would be a bug elsewhere in
QEMU), then maybe we should have
switch (fid_type) {
case P9_FID_DIR:
return dirfd(fs->dir.stream);
case P9_FID_FILE:
return fs->fd;
default:
g_assert_not_reached();
}
? That's the standard way we write "this isn't possible" and
avoid it turning into doing weird stuff if it ever does happen.
> , 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.
I'll do that.
thanks
-- PMM
