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

[Ján Tomko]

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 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>

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190415/7e989616/attachment-0001.sig>