The commit summary reads like you're introducing the segfaults; how about: rpc: fix cleanup in virNetTLSContextNew On Mon, Apr 15, 2019 at 10:21:13AM +0100, Daniel P. Berrangé wrote:
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:
This sentence is unnecessary
https://bugzilla.redhat.com/show_bug.cgi?id=1699062 Signed-off-by: Adrian Brzezinski <redhat@xxxxxxx> --- 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>
Please put the news update in a separate commit - otherwise developers backporting the patch into downstream distros will pretty much always have a conflict in this file.
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);
I'll just add that VIR_FREE can safely be called on NULL pointers and it sets the pointer to NULL after freeing it. Jano
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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
