* [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic @ 2014-10-29 10:52 ` arei.gonglei 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* [Qemu-devel] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic @ 2014-10-29 10:52 ` arei.gonglei 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic 2014-10-29 10:52 ` [Qemu-devel] " arei.gonglei @ 2014-10-30 8:03 ` Michael Tokarev -1 siblings, 0 replies; 6+ 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] 6+ 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 ` Michael Tokarev 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic 2014-10-30 8:03 ` [Qemu-devel] " Michael Tokarev @ 2014-10-30 10:51 ` Gonglei -1 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic @ 2014-10-30 10:51 ` Gonglei 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2014-10-30 10:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-29 10:52 [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic arei.gonglei 2014-10-29 10:52 ` [Qemu-devel] " arei.gonglei 2014-10-30 8:03 ` [Qemu-trivial] " Michael Tokarev 2014-10-30 8:03 ` [Qemu-devel] " Michael Tokarev 2014-10-30 10:51 ` Gonglei 2014-10-30 10:51 ` [Qemu-devel] " Gonglei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.