Re: [PULL 037/108] virtiofsd: passthrough_ll: add dirp_map to hide lo_dirp pointers

[Peter Maydell]

On Thu, 23 Jan 2020 at 19:41, Dr. David Alan Gilbert (git)
<address@hidden> wrote:
>
> From: Stefan Hajnoczi <address@hidden>
>
> Do not expose lo_dirp pointers to clients.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  tools/virtiofsd/passthrough_ll.c | 103 +++++++++++++++++++++++--------
>  1 file changed, 76 insertions(+), 27 deletions(-)

Hi; Coverity reports (CID 1421933) an issue introduced
by this patch:

> @@ -873,6 +881,7 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino,
>      struct lo_data *lo = lo_data(req);
>      struct lo_dirp *d;
>      int fd;
> +    ssize_t fh;
>
>      d = calloc(1, sizeof(struct lo_dirp));
>      if (d == NULL) {
> @@ -892,7 +901,14 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino,
>      d->offset = 0;
>      d->entry = NULL;
>
> -    fi->fh = (uintptr_t)d;
> +    pthread_mutex_lock(&lo->mutex);
> +    fh = lo_add_dirp_mapping(req, d);
> +    pthread_mutex_unlock(&lo->mutex);
> +    if (fh == -1) {
> +        goto out_err;
> +    }

This change introduces a new code path which
leads to out_err that can be taken after the
call to fdopendir(fd) succeeds...

> +
> +    fi->fh = fh;
>      if (lo->cache == CACHE_ALWAYS) {
>          fi->keep_cache = 1;
>      }
> @@ -903,6 +919,9 @@ out_errno:
>      error = errno;
>  out_err:
>      if (d) {
> +        if (d->dp) {
> +            closedir(d->dp);
> +        }
>          if (fd != -1) {
>              close(fd);
>          }

...but here we close(fd), which is an error if
fdopendir() succeeded, because that function takes
over ownership of the fd it is passed and we should
make no more calls on it.

An easy fix would be to add 'fd = -1;' after the call
to fdopendir() has succeeded (but that would probably
deserve a comment explaining why it's being done); or
you might prefer to separate out the error flows so
we only call close() for error paths before the fdopendir()
succeeds and only call closedir() afterwards. Or
you could make the error path

   if (d->dp) {
       closedir(d->dp);
   } else if (fd != -1) {
       close(fd);
   }

thanks
-- PMM