Re: [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic

[Michael Tokarev]

29.10.2014 13:52, address@hidden wrote:
> From: Gonglei <address@hidden>
> 
> If connect() return false, the sockfd will leak,
> meanwhile proxy_init() can't check the return value
> of connect_namedsocket(), maybe cause unpredictable
> results.
> 
> Let's move the sock_id check logic out, which can
> check both if and else statements.
> 
> Signed-off-by: Gonglei <address@hidden>
> ---
>  hw/9pfs/virtio-9p-proxy.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
> index b57966d..1c3aa5f 100644
> --- a/hw/9pfs/virtio-9p-proxy.c
> +++ b/hw/9pfs/virtio-9p-proxy.c
> @@ -1112,6 +1112,7 @@ static int connect_namedsocket(const char *path)
>      size = strlen(helper.sun_path) + sizeof(helper.sun_family);
>      if (connect(sockfd, (struct sockaddr *)&helper, size) < 0) {
>          fprintf(stderr, "socket error\n");
> +        close(sockfd);
>          return -1;
>      }
>  
> @@ -1152,11 +1153,12 @@ static int proxy_init(FsContext *ctx)
>          sock_id = connect_namedsocket(ctx->fs_root);
>      } else {
>          sock_id = atoi(ctx->fs_root);
> -        if (sock_id < 0) {
> -            fprintf(stderr, "socket descriptor not initialized\n");
> -            g_free(proxy);
> -            return -1;
> -        }
> +    }
> +
> +    if (sock_id < 0) {
> +        fprintf(stderr, "socket descriptor not initialized\n");
> +        g_free(proxy);
> +        return -1;
>      }
>      g_free(ctx->fs_root);
>      ctx->fs_root = NULL;


Um. I'm applying 2 patches instead of this one.  First:



virtio-9p-proxy: Fix sockfd leak

If connect() in connect_namedsocket() return false, the sockfd will leak.
Plug it.

Signed-off-by: Michael Tokarev <address@hidden>
Signed-off-by: Gonglei <address@hidden>

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index b57966d..e6bbb06 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -1112,6 +1112,7 @@ static int connect_namedsocket(const char *path)
     size = strlen(helper.sun_path) + sizeof(helper.sun_family);
     if (connect(sockfd, (struct sockaddr *)&helper, size) < 0) {
         fprintf(stderr, "socket error\n");
+        close(sockfd);
         return -1;
     }



And second.  Note the slight change in logic and error messages
compared with your version - there's no need to print error
message twice if connect_namedsocket() returned -1 (it already
printed error message).

virtio-9p-proxy: fix error return in proxy_init()

proxy_init() does not check the return value of connect_namedsocket(),
fix this by rearranging code a little bit.

Signed-off-by: Michael Tokarev <address@hidden>

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index e6bbb06..2ec211b 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -1155,10 +1155,12 @@ static int proxy_init(FsContext *ctx)
         sock_id = atoi(ctx->fs_root);
         if (sock_id < 0) {
             fprintf(stderr, "socket descriptor not initialized\n");
-            g_free(proxy);
-            return -1;
         }
     }
+    if (sock_id < 0) {
+        g_free(proxy);
+        return -1;
+    }
     g_free(ctx->fs_root);
     ctx->fs_root = NULL;


And I'll immediately send another followup patch to improve
error messages in connect_namedsocket(), -- these are awful.

/mjt