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

[Laine Stump]

On 02/09/2017 09:21 AM, Marc Hartmayer wrote:
> On Thu, Feb 09, 2017 at 03:13 PM +0100, Marc Hartmayer <mhartmay at 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 at linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list