From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XjnKt-0006bm-0s for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:52:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XjnKo-0007mv-LF for qemu-devel@nongnu.org; Thu, 30 Oct 2014 06:52:10 -0400 Message-ID: <54521836.3070701@huawei.com> Date: Thu, 30 Oct 2014 18:51:34 +0800 From: Gonglei MIME-Version: 1.0 References: <1414579937-1064-1-git-send-email-arei.gonglei@huawei.com> <5451F0CC.3030100@msgid.tls.msk.ru> In-Reply-To: <5451F0CC.3030100@msgid.tls.msk.ru> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH] virtio-9p-proxy: Fix sockfd leak and modify the check logic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >> >> 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 >> --- >> 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 > Signed-off-by: Gonglei > > 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 > > 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