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

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(a)adrb.pl&gt; > --- > 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