1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/4] Fixes for segmentation faults
  4. View patch

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

Marc Hartmayer posted 4 patches 9 years ago
[libvirt] [PATCH 4/4] rpc: Fix potentially segfaults
Posted by Marc Hartmayer 9 years ago 
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);
+    }
 
 #if WITH_GNUTLS
     virObjectUnref(svc->tls);
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] rpc: Fix potentially segfaults
Posted by Marc Hartmayer 9 years ago 
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)?

> +    }
>
>  #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
Re: [libvirt] [PATCH 4/4] rpc: Fix potentially segfaults
Posted by Laine Stump 9 years ago 
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] rpc: Fix potentially segfaults
Posted by Marc Hartmayer 8 years, 12 months ago 
On Thu, Feb 09, 2017 at 08:17 PM +0100, Laine Stump <laine@laine.org> wrote:
> 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).

No worries from my side so you can change it - thanks.

--
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
Re: [libvirt] [PATCH 4/4] rpc: Fix potentially segfaults
Posted by Laine Stump 9 years ago 
On 02/09/2017 09:13 AM, Marc Hartmayer 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);
> +    }

re: your other message - we shouldn't need to check for svc->socks != 
NULL here (see my response to your self-response). And I *think* we 
don't need to worry about settinc svc->nsocks = 0 here, since the 
dispose function can only be called once per object.

Do you see any problem with me removing the "if (svc->socks)" around the 
for loop and VIR_FREE()? If not, I'll do that and push the result. (ACK, 
assuming that change is okay with you)


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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.