fdopendir(3) takes ownership of the file descriptor.  The presence of
the lo_dirp->fd field could lead to someone incorrectly adding a
close(d->fd) cleanup call in the future.

Do not store the file descriptor in struct lo_dirp since it is unused.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/passthrough_ll.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index c1500e092d..ad3abdd532 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1293,7 +1293,6 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
 }
 
 struct lo_dirp {
-	int fd;
 	DIR *dp;
 	struct dirent *entry;
 	off_t offset;
@@ -1319,16 +1318,17 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
 	struct lo_data *lo = lo_data(req);
 	struct lo_dirp *d;
 	ssize_t fh;
+	int fd = -1;
 
 	d = calloc(1, sizeof(struct lo_dirp));
 	if (d == NULL)
 		goto out_err;
 
-	d->fd = openat(lo_fd(req, ino), ".", O_RDONLY);
-	if (d->fd == -1)
+	fd = openat(lo_fd(req, ino), ".", O_RDONLY);
+	if (fd == -1)
 		goto out_errno;
 
-	d->dp = fdopendir(d->fd);
+	d->dp = fdopendir(fd);
 	if (d->dp == NULL)
 		goto out_errno;
 
@@ -1348,11 +1348,12 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
 out_errno:
 	error = errno;
 out_err:
+	if (fd != -1) {
+		close(fd);
+	}
 	if (d) {
 		if (d->dp)
 			closedir(d->dp);
-		if (d->fd != -1)
-			close(d->fd);
 		free(d);
 	}
 	fuse_reply_err(req, error);
-- 
2.21.0

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> fdopendir(3) takes ownership of the file descriptor.  The presence of
> the lo_dirp->fd field could lead to someone incorrectly adding a
> close(d->fd) cleanup call in the future.
> 
> Do not store the file descriptor in struct lo_dirp since it is unused.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Thanks, applied; note:
  a) This looks like it can go into upstream libfuse
  b) I think we're probably leaking DIR *'s if we do an lo_shutdown;
I've added that to my todo

> ---
>  contrib/virtiofsd/passthrough_ll.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index c1500e092d..ad3abdd532 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1293,7 +1293,6 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
>  }
>  
>  struct lo_dirp {
> -	int fd;
>  	DIR *dp;
>  	struct dirent *entry;
>  	off_t offset;
> @@ -1319,16 +1318,17 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
>  	struct lo_data *lo = lo_data(req);
>  	struct lo_dirp *d;
>  	ssize_t fh;
> +	int fd = -1;
>  
>  	d = calloc(1, sizeof(struct lo_dirp));
>  	if (d == NULL)
>  		goto out_err;
>  
> -	d->fd = openat(lo_fd(req, ino), ".", O_RDONLY);
> -	if (d->fd == -1)
> +	fd = openat(lo_fd(req, ino), ".", O_RDONLY);
> +	if (fd == -1)
>  		goto out_errno;
>  
> -	d->dp = fdopendir(d->fd);
> +	d->dp = fdopendir(fd);
>  	if (d->dp == NULL)
>  		goto out_errno;
>  
> @@ -1348,11 +1348,12 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
>  out_errno:
>  	error = errno;
>  out_err:
> +	if (fd != -1) {
> +		close(fd);
> +	}
>  	if (d) {
>  		if (d->dp)
>  			closedir(d->dp);
> -		if (d->fd != -1)
> -			close(d->fd);
>  		free(d);
>  	}
>  	fuse_reply_err(req, error);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Wed, Jul 31, 2019 at 06:27:16PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > fdopendir(3) takes ownership of the file descriptor.  The presence of
> > the lo_dirp->fd field could lead to someone incorrectly adding a
> > close(d->fd) cleanup call in the future.
> > 
> > Do not store the file descriptor in struct lo_dirp since it is unused.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 
> Thanks, applied; note:
>   a) This looks like it can go into upstream libfuse
>   b) I think we're probably leaking DIR *'s if we do an lo_shutdown;
> I've added that to my todo

FUSE_DESTROY is current broken, it leaks resources and is not
thread-safe.  It's something I'll tackle in multithreading preparation
part 3.

Stefan