[libvirt] [PATCH] rpc: Segfaults and memory leak in virNetTLSContextNew function

[Daniel P. Berrangé]

On Fri, Apr 12, 2019 at 06:10:49PM +0200, Adrian Brzezinski wrote:
> Failed new gnutls context allocations in virNetTLSContextNew function
> results in double free and segfault. Occasional memory leaks may also
> occur. You can read detailed description at:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1699062
> 
> Signed-off-by: Adrian Brzezinski <redhat at adrb.pl>
> ---
>  docs/news.xml              | 10 ++++++++++
>  src/rpc/virnettlscontext.c |  6 ++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 21807f2..f6157ec 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -350,6 +350,16 @@
>      <section title="Bug fixes">
>        <change>
>          <summary>
> +          rpc: Segfaults and memory leak in virNetTLSContextNew function
> +        </summary>
> +        <description>
> +          Failed new gnutls context allocations in virNetTLSContextNew function
> +          results in double free and segfault. Occasional memory leaks may also
> +          occur.
> +        </description>
> +      </change>
> +      <change>
> +        <summary>
>            qemu: Use CAP_DAC_OVERRIDE during QEMU capabilities probing
>          </summary>
>          <description>
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 72e9ed9..8f6ec8f 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -703,14 +703,14 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert,
>          return NULL;
>  
>      if (VIR_STRDUP(ctxt->priority, priority) < 0)
> -        goto error;
> +        goto ctxt_init_error;
>  
>      err = gnutls_certificate_allocate_credentials(&ctxt->x509cred);
>      if (err) {
>          virReportError(VIR_ERR_SYSTEM_ERROR,
>                         _("Unable to allocate x509 credentials: %s"),
>                         gnutls_strerror(err));
> -        goto error;
> +        goto ctxt_init_error;
>      }
>  
>      if (sanityCheckCert &&
> @@ -759,6 +759,8 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert,
>      if (isServer)
>          gnutls_dh_params_deinit(ctxt->dhParams);
>      gnutls_certificate_free_credentials(ctxt->x509cred);
> + ctxt_init_error:

Having multiple label targets is not an attractive pattern. The core
problem here is that gnutls_certificate_free_credentials will
unconditionally dereference the credentials struct without checking
if it is NULL. We can avoid this by just adding  a check

    if (ctxt->x509cred)
        gnutls_certificate_free_credentials(ctxt->x509cred);

> +    if (ctxt->priority) VIR_FREE(ctxt->priority);
>      VIR_FREE(ctxt);
>      return NULL;
>  }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|