[PATCH] hw/9pfs: Check for error return from local_fid_fd()

[PATCH] hw/9pfs: Check for error return from local_fid_fd()

[Peter Maydell]

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 <peter.maydell@linaro.org>
---
 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);
     if (err) {
         return err;
@@ -1055,6 +1058,9 @@ static int local_ftruncate(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
 {
     int fd = local_fid_fd(fid_type, fs);
 
+    if (fd < 0) {
+        return fd;
+    }
     return ftruncate(fd, size);
 }
 
@@ -1113,6 +1119,9 @@ static int local_futimens(FsContext *s, int fid_type, V9fsFidOpenState *fs,
 {
     int fd = local_fid_fd(fid_type, fs);
 
+    if (fd < 0) {
+        return fd;
+    }
     return qemu_futimens(fd, times);
 }
 
@@ -1196,6 +1205,9 @@ static int local_fsync(FsContext *ctx, int fid_type,
 {
     int fd = local_fid_fd(fid_type, fs);
 
+    if (fd < 0) {
+        return fd;
+    }
     if (datasync) {
         return qemu_fdatasync(fd);
     } else {
-- 
2.43.0

Re: [PATCH] hw/9pfs: Check for error return from local_fid_fd()

[Michael Tokarev]

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 <peter.maydell@linaro.org>
> ---
>   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.

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.

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.

Thanks,

/mjt

Re: [PATCH] hw/9pfs: Check for error return from local_fid_fd()

[Peter Maydell]

On Fri, 3 Jul 2026 at 14:53, Michael Tokarev <mjt@tls.msk.ru> 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 <peter.maydell@linaro.org>
> > ---
> >   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

Re: [PATCH] hw/9pfs: Check for error return from local_fid_fd()

[Christian Schoenebeck]

On Friday, 3 July 2026 15:25:28 CEST 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 <peter.maydell@linaro.org>
> ---
>  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;
> +    }

Officially only -1 is an invalid file descriptor, and if you look at this file 
we therefore check file descriptors exactly against -1 for that reason.

For the Linux kernel the entire negative range is invalid for file 
descriptors, but keep in mind that 9pfs in QEMU also supports FreeBSD and 
macOS hosts and we have Windows host patches lurking around where things are 
not for certain, at least not for me. So I would not break that assumption.

You might also consider adding error_report_once() in local_fid_fd() as this 
would be an unexpected case, even though gracefully handled already.

/Christian