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: > > 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!).
Oops, yes. I usually try to find a place to reply by searching the mailing list, and I picked the wrong result by mistake here :-) > 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. That part is fine. What Coverity is warning about is the callsites: > > > + > > > > > > 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. thanks -- PMM
