On Fri, 3 Jul 2026 at 14:53, Michael Tokarev <[email protected]> wrote: > > On 03.07.2026 16:25, Peter Maydell wrote: > > The local_fid_fd() function can fail (returning -1 if dirfd() fails); > > check for this explicitly rather than relying on something later on > > failing because it was passed a -1 file descriptor. > > > > This was flagged up by Coverity: CID 1660923, 1660924, 1660925 > > > > Signed-off-by: Peter Maydell <[email protected]> > > --- > > hw/9pfs/9p-local.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index ee592b62f8..8f27c562b5 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -788,6 +788,9 @@ static int local_fstat(FsContext *fs_ctx, int fid_type, > > { > > int err, fd = local_fid_fd(fid_type, fs); > > > > + if (fd < 0) { > > + return fd; > > + } > > err = fstat(fd, stbuf); > > I think this is a wrong approach - if we're going to report error due to > a failed (local_)fstat call, we'll use errno which is not set in this > codepath. > > Instead we should check fd earlier.
There is no earlier point. local_fid_fd() calls dirfd(). dirfd() can fail, returning -1, so local_fid_fd() can fail, returning -1, so we should check it. > In this particular context, we don't check possible error return from > dirfd() - which should not return error in our case, as we know we have > valid opendir() handle (unless we really messed up our memory layout, > but in this case we've much bigger problem). > > Mayve > > assert(dirft() >= 0) > > in local_fid_fd() will be better. I'm not generally a fan of "assert that this system or library call can't fail": it's making a claim about code we don't control and that doesn't document itself as providing that guarantee. > In general, seeing code like the suggested in this patch, prompts me > thinking that fd can normally be negative in this context, - and handle > this situation in other cases too. But it really can't, and if it is, > it's an abnormal situation. This is just local_fid_fd() having the usual Unix style "returns an fd or -1 on error" semantics, same as open(2) does. thanks -- PMM
