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

[Gonglei <arei.gonglei@huawei.com>]

On 2014/10/30 16:03, Michael Tokarev wrote:

> 29.10.2014 13:52, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> 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 <arei.gonglei@huawei.com>
>> ---
>>  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:
> 
> 

Ok, Thanks for your work.

Best regards,
-Gonglei

> 
> virtio-9p-proxy: Fix sockfd leak
> 
> If connect() in connect_namedsocket() return false, the sockfd will leak.
> Plug it.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> 
> 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 <mjt@tls.msk.ru>
> 
> 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