* [Qemu-devel] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic
@ 2014-10-29 10:52 arei.gonglei
2014-10-30 8:03 ` [Qemu-devel] [Qemu-trivial] " Michael Tokarev
0 siblings, 1 reply; 3+ messages in thread
From: arei.gonglei @ 2014-10-29 10:52 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Gonglei, weidong.huang, aneesh.kumar, mst
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;
--
1.7.12.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic
2014-10-29 10:52 [Qemu-devel] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic arei.gonglei
@ 2014-10-30 8:03 ` Michael Tokarev
2014-10-30 10:51 ` Gonglei
0 siblings, 1 reply; 3+ messages in thread
From: Michael Tokarev @ 2014-10-30 8:03 UTC (permalink / raw)
To: arei.gonglei, qemu-devel; +Cc: qemu-trivial, weidong.huang, aneesh.kumar, mst
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:
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic
2014-10-30 8:03 ` [Qemu-devel] [Qemu-trivial] " Michael Tokarev
@ 2014-10-30 10:51 ` Gonglei
0 siblings, 0 replies; 3+ messages in thread
From: Gonglei @ 2014-10-30 10:51 UTC (permalink / raw)
To: Michael Tokarev
Cc: qemu-trivial@nongnu.org, mst@redhat.com, Huangweidong (C),
qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-30 10:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-29 10:52 [Qemu-devel] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic arei.gonglei
2014-10-30 8:03 ` [Qemu-devel] [Qemu-trivial] " Michael Tokarev
2014-10-30 10:51 ` Gonglei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).