[Virtio-fs] [PATCH v2 0/5] virtiofsd: leak fixes

[Virtio-fs] [PATCH v2 0/5] virtiofsd: 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  |  7 +++++++
 contrib/virtiofsd/fuse_virtio.c    |  7 +++++++
 contrib/virtiofsd/fuse_virtio.h    |  2 +-
 contrib/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++++--------------
 4 files changed, 37 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[Virtio-fs] [PATCH v2 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 | 7 +++++++
 contrib/virtiofsd/fuse_virtio.c   | 7 +++++++
 contrib/virtiofsd/fuse_virtio.h   | 2 +-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
index 0fc2880..c460c4c 100644
--- a/contrib/virtiofsd/fuse_lowlevel.c
+++ b/contrib/virtiofsd/fuse_lowlevel.c
@@ -2557,6 +2557,13 @@ void fuse_session_destroy(struct fuse_session *se)
 	free(se->cuse_data);
 	if (se->fd != -1)
 		close(se->fd);
+
+        if (se->vu_socket_path) {
+                virtio_session_close(se);
+                free(se->vu_socket_path);
+                se->vu_socket_path = NULL;
+        }
+
 	free(se);
 }
 
diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c
index cb46558..ed6ba57 100644
--- a/contrib/virtiofsd/fuse_virtio.c
+++ b/contrib/virtiofsd/fuse_virtio.c
@@ -886,6 +886,13 @@ int virtio_session_mount(struct fuse_session *se)
         return 0;
 }
 
+void virtio_session_close(struct fuse_session *se)
+{
+        close(se->vu_socketfd);
+        free(se->virtio_dev);
+        se->virtio_dev = NULL;
+}
+
 int64_t fuse_virtio_map(fuse_req_t req, VhostUserFSSlaveMsg *msg, int fd)
 {
         if (!req->se->virtio_dev) return -ENODEV;
diff --git a/contrib/virtiofsd/fuse_virtio.h b/contrib/virtiofsd/fuse_virtio.h
index 19c468e..e602298 100644
--- a/contrib/virtiofsd/fuse_virtio.h
+++ b/contrib/virtiofsd/fuse_virtio.h
@@ -19,7 +19,7 @@
 struct fuse_session;
 
 int virtio_session_mount(struct fuse_session *se);
-
+void virtio_session_close(struct fuse_session *se);
 int virtio_loop(struct fuse_session *se);
 
 
-- 
1.8.3.1

[Virtio-fs] [PATCH v2 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 "/".

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 b58708f..959d74d 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2269,9 +2269,8 @@ 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");
-
 	} else {
-		lo.source = "/";
+		lo.source = strdup("/");
 	}
 	lo.root.is_symlink = false;
 	if (!lo.timeout_set) {
@@ -2333,5 +2332,7 @@ err_out1:
 	if (lo.root.fd >= 0)
 		close(lo.root.fd);
 
+        free((char *)lo.source);
+
 	return ret ? 1 : 0;
 }
-- 
1.8.3.1

[Virtio-fs] [PATCH v2 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.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
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 959d74d..57508df 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2321,6 +2321,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 v2 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.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
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 57508df..73484d1 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2242,7 +2242,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();
@@ -2257,7 +2258,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 v2 5/5] virtiofsd: add helper for lo_data cleanup

[Liu Bo]

This offers an helper function for lo_data's cleanup.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 contrib/virtiofsd/passthrough_ll.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 73484d1..067a110 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2205,6 +2205,23 @@ static gboolean lo_key_equal(gconstpointer a, gconstpointer b)
                la->dev == lb->dev;
 }
 
+static void fuse_lo_data_cleanup(struct lo_data *lo)
+{
+        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);
+
+        free((char *)lo->source);
+}
 
 int main(int argc, char *argv[])
 {
@@ -2322,20 +2339,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);
-
-        free((char *)lo.source);
+        fuse_lo_data_cleanup(&lo);
 
 	return ret ? 1 : 0;
 }
-- 
1.8.3.1

Re: [Virtio-fs] [PATCH v2 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>

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

> ---
>  contrib/virtiofsd/fuse_lowlevel.c | 7 +++++++
>  contrib/virtiofsd/fuse_virtio.c   | 7 +++++++
>  contrib/virtiofsd/fuse_virtio.h   | 2 +-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> index 0fc2880..c460c4c 100644
> --- a/contrib/virtiofsd/fuse_lowlevel.c
> +++ b/contrib/virtiofsd/fuse_lowlevel.c
> @@ -2557,6 +2557,13 @@ void fuse_session_destroy(struct fuse_session *se)
>  	free(se->cuse_data);
>  	if (se->fd != -1)
>  		close(se->fd);
> +
> +        if (se->vu_socket_path) {
> +                virtio_session_close(se);
> +                free(se->vu_socket_path);
> +                se->vu_socket_path = NULL;
> +        }
> +
>  	free(se);
>  }
>  
> diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c
> index cb46558..ed6ba57 100644
> --- a/contrib/virtiofsd/fuse_virtio.c
> +++ b/contrib/virtiofsd/fuse_virtio.c
> @@ -886,6 +886,13 @@ int virtio_session_mount(struct fuse_session *se)
>          return 0;
>  }
>  
> +void virtio_session_close(struct fuse_session *se)
> +{
> +        close(se->vu_socketfd);
> +        free(se->virtio_dev);
> +        se->virtio_dev = NULL;
> +}
> +
>  int64_t fuse_virtio_map(fuse_req_t req, VhostUserFSSlaveMsg *msg, int fd)
>  {
>          if (!req->se->virtio_dev) return -ENODEV;
> diff --git a/contrib/virtiofsd/fuse_virtio.h b/contrib/virtiofsd/fuse_virtio.h
> index 19c468e..e602298 100644
> --- a/contrib/virtiofsd/fuse_virtio.h
> +++ b/contrib/virtiofsd/fuse_virtio.h
> @@ -19,7 +19,7 @@
>  struct fuse_session;
>  
>  int virtio_session_mount(struct fuse_session *se);
> -
> +void virtio_session_close(struct fuse_session *se);
>  int virtio_loop(struct fuse_session *se);
>  
>  
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v2 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 "/".
> 
> 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 b58708f..959d74d 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -2269,9 +2269,8 @@ 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");
> -
>  	} else {
> -		lo.source = "/";
> +		lo.source = strdup("/");
>  	}
>  	lo.root.is_symlink = false;
>  	if (!lo.timeout_set) {
> @@ -2333,5 +2332,7 @@ err_out1:
>  	if (lo.root.fd >= 0)
>  		close(lo.root.fd);
>  
> +        free((char *)lo.source);
> +
>  	return ret ? 1 : 0;
>  }
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v2 0/5] virtiofsd: leak fixes

[Dr. David Alan Gilbert]

* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> Patch 1-4 fixes memory/fd/hashmap leakage.
> Patch 5 is a cleanup.

I've just pushed a new -dev branch that includes this and a bunch of
other fixes.

Dave

> 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  |  7 +++++++
>  contrib/virtiofsd/fuse_virtio.c    |  7 +++++++
>  contrib/virtiofsd/fuse_virtio.h    |  2 +-
>  contrib/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++++--------------
>  4 files changed, 37 insertions(+), 15 deletions(-)
> 
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK