1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] rpc: virNetClientNew: fix socket leak on error path

Nikolay Shirokovskiy posted 1 patch 5 years, 3 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1547800719-619233-1-git-send-email-nshirokovskiy@virtuozzo.com
src/rpc/virnetclient.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
[libvirt] [PATCH] rpc: virNetClientNew: fix socket leak on error path
Posted by Nikolay Shirokovskiy 5 years, 3 months ago 
if virNetClientNew finishes with error before sock is set
to client object then sock does not get unrefed. This is
unexpected by function clients like virNetClientNewUNIX.
Let's make sure sock gets unrefed on any error path.

Next some clients like virNetClientNewLibSSH2 try to unref
sock on virNetClientNew errors. This is not correct even
before this patch because in some cases virNetClientNew
unrefed sock on error path by itself. Let's give up
sock managment to virNetClientNew entirely.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/rpc/virnetclient.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 2aced79..b7ffdcb 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -299,7 +299,7 @@ static virNetClientPtr virNetClientNew(virNetSocketPtr sock,
     int wakeupFD[2] = { -1, -1 };
 
     if (virNetClientInitialize() < 0)
-        return NULL;
+        goto error;
 
     if (pipe2(wakeupFD, O_CLOEXEC) < 0) {
         virReportSystemError(errno, "%s",
@@ -311,6 +311,7 @@ static virNetClientPtr virNetClientNew(virNetSocketPtr sock,
         goto error;
 
     client->sock = sock;
+    sock = NULL;
     client->wakeupReadFD = wakeupFD[0];
     client->wakeupSendFD = wakeupFD[1];
     wakeupFD[0] = wakeupFD[1] = -1;
@@ -327,6 +328,7 @@ static virNetClientPtr virNetClientNew(virNetSocketPtr sock,
     VIR_FORCE_CLOSE(wakeupFD[0]);
     VIR_FORCE_CLOSE(wakeupFD[1]);
     virObjectUnref(client);
+    virObjectUnref(sock);
     return NULL;
 }
 
@@ -513,7 +515,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 
     if (!(ret = virNetClientNew(sock, NULL)))
         goto cleanup;
-    sock = NULL;
 
  cleanup:
     VIR_FREE(command);
@@ -522,7 +523,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
     VIR_FREE(homedir);
     VIR_FREE(confdir);
     VIR_FREE(nc);
-    virObjectUnref(sock);
     return ret;
 
  no_memory:
@@ -619,7 +619,6 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
 
     if (!(ret = virNetClientNew(sock, NULL)))
         goto cleanup;
-    sock = NULL;
 
  cleanup:
     VIR_FREE(command);
@@ -628,7 +627,6 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
     VIR_FREE(homedir);
     VIR_FREE(confdir);
     VIR_FREE(nc);
-    virObjectUnref(sock);
     return ret;
 
  no_memory:
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: virNetClientNew: fix socket leak on error path
Posted by Daniel P. Berrangé 5 years, 3 months ago 
On Fri, Jan 18, 2019 at 11:38:39AM +0300, Nikolay Shirokovskiy wrote:
> if virNetClientNew finishes with error before sock is set
> to client object then sock does not get unrefed. This is
> unexpected by function clients like virNetClientNewUNIX.
> Let's make sure sock gets unrefed on any error path.
> 
> Next some clients like virNetClientNewLibSSH2 try to unref
> sock on virNetClientNew errors. This is not correct even
> before this patch because in some cases virNetClientNew
> unrefed sock on error path by itself. Let's give up
> sock managment to virNetClientNew entirely.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/rpc/virnetclient.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: virNetClientNew: fix socket leak on error path
Posted by Nikolay Shirokovskiy 5 years, 3 months ago 

On 18.01.2019 13:32, Daniel P. Berrangé wrote:
> On Fri, Jan 18, 2019 at 11:38:39AM +0300, Nikolay Shirokovskiy wrote:
>> if virNetClientNew finishes with error before sock is set
>> to client object then sock does not get unrefed. This is
>> unexpected by function clients like virNetClientNewUNIX.
>> Let's make sure sock gets unrefed on any error path.
>>
>> Next some clients like virNetClientNewLibSSH2 try to unref
>> sock on virNetClientNew errors. This is not correct even
>> before this patch because in some cases virNetClientNew
>> unrefed sock on error path by itself. Let's give up
>> sock managment to virNetClientNew entirely.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>>  src/rpc/virnetclient.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 

Thanks! Pushed.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.