[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] rpc: Fix potentially segfaults



On 02/09/2017 09:21 AM, Marc Hartmayer wrote:
On Thu, Feb 09, 2017 at 03:13 PM +0100, Marc Hartmayer <mhartmay linux vnet ibm com> wrote:
We have to allocate first and if, and only if, it was successful we
can set the count. A segfault has occurred in
virNetServerServiceNewPostExecRestart() when VIR_ALLOC_N(svc->socks,
n) has failed, but svc->nsocsk = n was already set. Thus
virObejectUnref(svc) was called and therefore it was possible that
virNetServerServiceDispose was called => segmentation fault.  For
safeness NULL pointer check were added in
virNetServerServiceDispose().

Signed-off-by: Marc Hartmayer <mhartmay linux vnet ibm com>
Reviewed-by: Boris Fiuczynski <fiuczy linux vnet ibm com>
Reviewed-by: Bjoern Walk <bwalk linux vnet ibm com>
---
  src/rpc/virnetserverservice.c | 20 +++++++++++---------
  1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index 1ef0636..006d041 100644
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -228,9 +228,9 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path,
      svc->tls = virObjectRef(tls);
  #endif

-    svc->nsocks = 1;
-    if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0)
+    if (VIR_ALLOC_N(svc->socks, 1) < 0)
          goto error;
+    svc->nsocks = 1;

      if (virNetSocketNewListenUNIX(path,
                                    mask,
@@ -289,9 +289,9 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd,
      svc->tls = virObjectRef(tls);
  #endif

-    svc->nsocks = 1;
-    if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0)
+    if (VIR_ALLOC_N(svc->socks, 1) < 0)
          goto error;
+    svc->nsocks = 1;

      if (virNetSocketNewListenFD(fd,
                                  &svc->socks[0]) < 0)
@@ -367,9 +367,9 @@ virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr obj
          goto error;
      }

-    svc->nsocks = n;
-    if (VIR_ALLOC_N(svc->socks, svc->nsocks) < 0)
+    if (VIR_ALLOC_N(svc->socks, n) < 0)
          goto error;
+    svc->nsocks = n;

      for (i = 0; i < svc->nsocks; i++) {
          virJSONValuePtr child = virJSONValueArrayGet(socks, i);
@@ -492,9 +492,11 @@ void virNetServerServiceDispose(void *obj)
      virNetServerServicePtr svc = obj;
      size_t i;

-    for (i = 0; i < svc->nsocks; i++)
-        virObjectUnref(svc->socks[i]);
-    VIR_FREE(svc->socks);
+    if (svc->socks) {
+        for (i = 0; i < svc->nsocks; i++)
+            virObjectUnref(svc->socks[i]);
+        VIR_FREE(svc->socks);
Should we set svc->nsocks = 0 instead of using a NULL-pointer check (or
do both)?

For consistency with other code, we should set svc->nsocks = 0. svc->socks should also be NULL if it's not pointing to anything valid, but as long as it is initialized to NULL (it is) and always freed with VIR_FREE() (it should be), then that will always be the case.

When checking at vir*Free() time, we don't need the "if (svc->socks)", since nsocks == 0 will prevent the for loop from being executed, and VIR_FREE() is a NOP if the pointer is NULL.

I think your patch is fine after removing the "if (svc->socks)" (the original problem was just that nsocks was set before we tried to allocate socks). Unless you see some other issue, I will make that one change and push it (I'll wait for word back from you though).


+    }

  #if WITH_GNUTLS
      virObjectUnref(svc->tls);
--
2.5.5

--
Beste Grüße / Kind regards
    Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]