[Virtio-fs] [PATCH 0/5] leak fixes

[Virtio-fs] [PATCH 0/5] leak fixes

[Liu Bo]

Patch 1-4 fixes memory/fd/hashmap leakage.
Patch 5 is a cleanup.

Liu Bo (5):
  virtiofsd: cleanup allocated resource in se
  virtiofsd: fix memory leak on lo.source
  virtiofsd: fix memory leak on lo.inodes hashmap
  virtiofsd: fix error handling in main()
  virtiofsd: add helper for lo_data cleanup

 contrib/virtiofsd/fuse_lowlevel.c  |  6 ++++++
 contrib/virtiofsd/passthrough_ll.c | 38 +++++++++++++++++++++++++-------------
 2 files changed, 31 insertions(+), 13 deletions(-)

-- 
1.8.3.1

[Virtio-fs] [PATCH 1/5] virtiofsd: cleanup allocated resource in se

[Liu Bo]

This cleans up unfreed resources in se on quiting, including
se->virtio_dev, se->vu_socket_path, se->vu_socketfd.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 contrib/virtiofsd/fuse_lowlevel.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
index 0fc2880..de700ad 100644
--- a/contrib/virtiofsd/fuse_lowlevel.c
+++ b/contrib/virtiofsd/fuse_lowlevel.c
@@ -2557,6 +2557,12 @@ void fuse_session_destroy(struct fuse_session *se)
 	free(se->cuse_data);
 	if (se->fd != -1)
 		close(se->fd);
+
+        free(se->virtio_dev);
+        se->virtio_dev = NULL;
+        free(se->vu_socket_path);
+        close(se->vu_socketfd);
+
 	free(se);
 }
 
-- 
1.8.3.1

[Virtio-fs] [PATCH 2/5] virtiofsd: fix memory leak on lo.source

[Liu Bo]

valgrind reported that lo.source is leaked on quiting, but it was defined
as (const char*) as it may point to a const string "/".

This adds a check to free the allocated memory only.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 contrib/virtiofsd/passthrough_ll.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index b58708f..f348b16 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2217,6 +2217,7 @@ int main(int argc, char *argv[])
 	};
 	struct lo_map_elem *root_elem;
 	int ret = -1;
+        bool free_source = false;
 
 	/* Don't mask creation mode, kernel already did that */
 	umask(0);
@@ -2269,9 +2270,10 @@ int main(int argc, char *argv[])
 			err(1, "failed to stat source (\"%s\")", lo.source);
 		if (!S_ISDIR(stat.st_mode))
 			errx(1, "source is not a directory");
-
+                free_source = true;
 	} else {
 		lo.source = "/";
+                free_source = false;
 	}
 	lo.root.is_symlink = false;
 	if (!lo.timeout_set) {
@@ -2333,5 +2335,8 @@ err_out1:
 	if (lo.root.fd >= 0)
 		close(lo.root.fd);
 
+        if (free_source)
+                free((char *)lo.source);
+
 	return ret ? 1 : 0;
 }
-- 
1.8.3.1

[Virtio-fs] [PATCH 3/5] virtiofsd: fix memory leak on lo.inodes hashmap

[Liu Bo]

lo.inodes hashmap was not unref/destroy 'd on quiting, which was caught by valgrind.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 contrib/virtiofsd/passthrough_ll.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index f348b16..e7e19dc 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2324,6 +2324,8 @@ err_out2:
 err_out1:
 	fuse_opt_free_args(&args);
 
+        if (lo.inodes)
+                g_hash_table_destroy(lo.inodes);
 	lo_map_destroy(&lo.fd_map);
 	lo_map_destroy(&lo.dirp_map);
 	lo_map_destroy(&lo.ino_map);
-- 
1.8.3.1

[Virtio-fs] [PATCH 4/5] virtiofsd: fix error handling in main()

[Liu Bo]

Neither fuse_parse_cmdline() nor fuse_opt_parse() goes to the right place
to do cleanup.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 contrib/virtiofsd/passthrough_ll.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index e7e19dc..649991e 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2243,7 +2243,8 @@ int main(int argc, char *argv[])
 	lo_map_init(&lo.fd_map);
 
 	if (fuse_parse_cmdline(&args, &opts) != 0)
-		return 1;
+		goto err_out1;
+
 	if (opts.show_help) {
 		printf("usage: %s [options]\n\n", argv[0]);
 		fuse_cmdline_help();
@@ -2258,7 +2259,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (fuse_opt_parse(&args, &lo, lo_opts, NULL)== -1)
-		return 1;
+		goto err_out1;
 
 	lo.debug = opts.debug;
 	if (lo.source) {
-- 
1.8.3.1

[Virtio-fs] [PATCH 5/5] virtiofsd: add helper for lo_data cleanup

[Liu Bo]

This offers an helper function for lo_data's cleanup.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 contrib/virtiofsd/passthrough_ll.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 649991e..73ca09f 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2205,6 +2205,24 @@ static gboolean lo_key_equal(gconstpointer a, gconstpointer b)
                la->dev == lb->dev;
 }
 
+static void fuse_lo_data_cleanup(struct lo_data *lo, bool free_source)
+{
+        if (lo->inodes)
+                g_hash_table_destroy(lo->inodes);
+	lo_map_destroy(&lo->fd_map);
+	lo_map_destroy(&lo->dirp_map);
+	lo_map_destroy(&lo->ino_map);
+
+	if (lo->proc_self_fd >= 0) {
+		close(lo->proc_self_fd);
+	}
+
+	if (lo->root.fd >= 0)
+		close(lo->root.fd);
+
+        if (free_source)
+                free((char *)lo->source);
+}
 
 int main(int argc, char *argv[])
 {
@@ -2325,21 +2343,7 @@ err_out2:
 err_out1:
 	fuse_opt_free_args(&args);
 
-        if (lo.inodes)
-                g_hash_table_destroy(lo.inodes);
-	lo_map_destroy(&lo.fd_map);
-	lo_map_destroy(&lo.dirp_map);
-	lo_map_destroy(&lo.ino_map);
-
-	if (lo.proc_self_fd >= 0) {
-		close(lo.proc_self_fd);
-	}
-
-	if (lo.root.fd >= 0)
-		close(lo.root.fd);
-
-        if (free_source)
-                free((char *)lo.source);
+        fuse_lo_data_cleanup(&lo, free_source);
 
 	return ret ? 1 : 0;
 }
-- 
1.8.3.1

Re: [Virtio-fs] [PATCH 1/5] virtiofsd: cleanup allocated resource in se

[Dr. David Alan Gilbert]

* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> This cleans up unfreed resources in se on quiting, including
> se->virtio_dev, se->vu_socket_path, se->vu_socketfd.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  contrib/virtiofsd/fuse_lowlevel.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> index 0fc2880..de700ad 100644
> --- a/contrib/virtiofsd/fuse_lowlevel.c
> +++ b/contrib/virtiofsd/fuse_lowlevel.c
> @@ -2557,6 +2557,12 @@ void fuse_session_destroy(struct fuse_session *se)
>  	free(se->cuse_data);
>  	if (se->fd != -1)
>  		close(se->fd);
> +
> +        free(se->virtio_dev);
> +        se->virtio_dev = NULL;

We don't touch virtio_dev or vu_socketfd anywhere else in fuse_lowlevel.c -
so we should probably do this cleanup in fuse_virtio.c, and then have
something like:

   if (se->vu_socket_path) {
     virtio_session_close(se);
     free(se->vu_socket_path);
     se->vu_socket_path = NULL;
   }


> +        free(se->vu_socket_path);
> +        close(se->vu_socketfd);
> +
>  	free(se);
>  }
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 2/5] virtiofsd: fix memory leak on lo.source

[Dr. David Alan Gilbert]

* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> valgrind reported that lo.source is leaked on quiting, but it was defined
> as (const char*) as it may point to a const string "/".
> 
> This adds a check to free the allocated memory only.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index b58708f..f348b16 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -2217,6 +2217,7 @@ int main(int argc, char *argv[])
>  	};
>  	struct lo_map_elem *root_elem;
>  	int ret = -1;
> +        bool free_source = false;
>  
>  	/* Don't mask creation mode, kernel already did that */
>  	umask(0);
> @@ -2269,9 +2270,10 @@ int main(int argc, char *argv[])
>  			err(1, "failed to stat source (\"%s\")", lo.source);
>  		if (!S_ISDIR(stat.st_mode))
>  			errx(1, "source is not a directory");
> -
> +                free_source = true;
>  	} else {
>  		lo.source = "/";
> +                free_source = false;

An easier way is just to do    lo.source = strdup("/")
and then we don't need the flag.

>  	}
>  	lo.root.is_symlink = false;
>  	if (!lo.timeout_set) {
> @@ -2333,5 +2335,8 @@ err_out1:
>  	if (lo.root.fd >= 0)
>  		close(lo.root.fd);
>  
> +        if (free_source)
> +                free((char *)lo.source);
> +
>  	return ret ? 1 : 0;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 3/5] virtiofsd: fix memory leak on lo.inodes hashmap

[Dr. David Alan Gilbert]

* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> lo.inodes hashmap was not unref/destroy 'd on quiting, which was caught by valgrind.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index f348b16..e7e19dc 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -2324,6 +2324,8 @@ err_out2:
>  err_out1:
>  	fuse_opt_free_args(&args);
>  
> +        if (lo.inodes)
> +                g_hash_table_destroy(lo.inodes);
>  	lo_map_destroy(&lo.fd_map);
>  	lo_map_destroy(&lo.dirp_map);
>  	lo_map_destroy(&lo.ino_map);

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

> -- 
> 1.8.3.1
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 4/5] virtiofsd: fix error handling in main()

[Dr. David Alan Gilbert]

* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> Neither fuse_parse_cmdline() nor fuse_opt_parse() goes to the right place
> to do cleanup.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

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

> ---
>  contrib/virtiofsd/passthrough_ll.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index e7e19dc..649991e 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -2243,7 +2243,8 @@ int main(int argc, char *argv[])
>  	lo_map_init(&lo.fd_map);
>  
>  	if (fuse_parse_cmdline(&args, &opts) != 0)
> -		return 1;
> +		goto err_out1;
> +
>  	if (opts.show_help) {
>  		printf("usage: %s [options]\n\n", argv[0]);
>  		fuse_cmdline_help();
> @@ -2258,7 +2259,7 @@ int main(int argc, char *argv[])
>  	}
>  
>  	if (fuse_opt_parse(&args, &lo, lo_opts, NULL)== -1)
> -		return 1;
> +		goto err_out1;
>  
>  	lo.debug = opts.debug;
>  	if (lo.source) {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 5/5] virtiofsd: add helper for lo_data cleanup

[Dr. David Alan Gilbert]

* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> This offers an helper function for lo_data's cleanup.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

That's ok, but will need updating when you do a new version of the
lo.source.

Dave

> ---
>  contrib/virtiofsd/passthrough_ll.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 649991e..73ca09f 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -2205,6 +2205,24 @@ static gboolean lo_key_equal(gconstpointer a, gconstpointer b)
>                 la->dev == lb->dev;
>  }
>  
> +static void fuse_lo_data_cleanup(struct lo_data *lo, bool free_source)
> +{
> +        if (lo->inodes)
> +                g_hash_table_destroy(lo->inodes);
> +	lo_map_destroy(&lo->fd_map);
> +	lo_map_destroy(&lo->dirp_map);
> +	lo_map_destroy(&lo->ino_map);
> +
> +	if (lo->proc_self_fd >= 0) {
> +		close(lo->proc_self_fd);
> +	}
> +
> +	if (lo->root.fd >= 0)
> +		close(lo->root.fd);
> +
> +        if (free_source)
> +                free((char *)lo->source);
> +}
>  
>  int main(int argc, char *argv[])
>  {
> @@ -2325,21 +2343,7 @@ err_out2:
>  err_out1:
>  	fuse_opt_free_args(&args);
>  
> -        if (lo.inodes)
> -                g_hash_table_destroy(lo.inodes);
> -	lo_map_destroy(&lo.fd_map);
> -	lo_map_destroy(&lo.dirp_map);
> -	lo_map_destroy(&lo.ino_map);
> -
> -	if (lo.proc_self_fd >= 0) {
> -		close(lo.proc_self_fd);
> -	}
> -
> -	if (lo.root.fd >= 0)
> -		close(lo.root.fd);
> -
> -        if (free_source)
> -                free((char *)lo.source);
> +        fuse_lo_data_cleanup(&lo, free_source);
>  
>  	return ret ? 1 : 0;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 1/5] virtiofsd: cleanup allocated resource in se

[Liu Bo]

On Thu, Jun 06, 2019 at 10:04:30AM +0100, Dr. David Alan Gilbert wrote:
> * Liu Bo (bo.liu@linux.alibaba.com) wrote:
> > This cleans up unfreed resources in se on quiting, including
> > se->virtio_dev, se->vu_socket_path, se->vu_socketfd.
> > 
> > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> > ---
> >  contrib/virtiofsd/fuse_lowlevel.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> > index 0fc2880..de700ad 100644
> > --- a/contrib/virtiofsd/fuse_lowlevel.c
> > +++ b/contrib/virtiofsd/fuse_lowlevel.c
> > @@ -2557,6 +2557,12 @@ void fuse_session_destroy(struct fuse_session *se)
> >  	free(se->cuse_data);
> >  	if (se->fd != -1)
> >  		close(se->fd);
> > +
> > +        free(se->virtio_dev);
> > +        se->virtio_dev = NULL;
> 
> We don't touch virtio_dev or vu_socketfd anywhere else in fuse_lowlevel.c -
> so we should probably do this cleanup in fuse_virtio.c, and then have
> something like:
> 
>    if (se->vu_socket_path) {
>      virtio_session_close(se);
>      free(se->vu_socket_path);
>      se->vu_socket_path = NULL;
>    }
>

I see, will fix it.  Thanks for the comments.

thanks,
-liubo

> 
> > +        free(se->vu_socket_path);
> > +        close(se->vu_socketfd);
> > +
> >  	free(se);
> >  }
> >  
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Virtio-fs mailing list
> > Virtio-fs@redhat.com
> > https://www.redhat.com/mailman/listinfo/virtio-fs
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK