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

[Laine Stump]

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 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);
> +    }

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)