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

[Marc Hartmayer]

On Thu, Feb 09, 2017 at 08:17 PM +0100, Laine Stump <laine at 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 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).

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