[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify t
From: |
Gonglei |
Subject: |
Re: [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic |
Date: |
Thu, 30 Oct 2014 18:51:34 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
On 2014/10/30 16:03, Michael Tokarev wrote:
> 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:
>
>
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 <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