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

Adrian Brzezinski posted 1 patch 9 weeks ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1555085449-1835-1-git-send-email-adrian.brzezinski@eo.pl
docs/news.xml              | 10 ++++++++++
src/rpc/virnettlscontext.c |  6 ++++--
2 files changed, 14 insertions(+), 2 deletions(-)

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

Posted by Adrian Brzezinski 9 weeks ago
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@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:
+    if (ctxt->priority) VIR_FREE(ctxt->priority);
     VIR_FREE(ctxt);
     return NULL;
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/2] rpc: cleanup in virNetTLSContextNew

Posted by Adrian Brzezinski 9 weeks ago
Changes:
- no new goto label
- passing ctxt->priority directly to VIR_FREE
- dedicated commit for news file

Adrian Brzezinski (2):
  rpc: cleanup in virNetTLSContextNew
  news: cleanup in virNetTLSContextNew

 docs/news.xml              | 10 ++++++++++
 src/rpc/virnettlscontext.c | 10 +++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/2] rpc: cleanup in virNetTLSContextNew

Posted by Adrian Brzezinski 9 weeks ago
Failed new gnutls context allocations in virNetTLSContextNew function
results in double free and segfault. Occasional memory leaks may also
occur.

Signed-off-by: Adrian Brzezinski <redhat@adrb.pl>
---
 src/rpc/virnettlscontext.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 72e9ed9..7b5d578 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -707,6 +707,12 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert,
 
     err = gnutls_certificate_allocate_credentials(&ctxt->x509cred);
     if (err) {
+       /* gnutls_certificate_credentials_t is complex structure with multiple
+        * internal memory allocatons that can go wrong, so make sure that
+        * reference is NULL.
+        */
+        ctxt->x509cred = NULL;
+
         virReportError(VIR_ERR_SYSTEM_ERROR,
                        _("Unable to allocate x509 credentials: %s"),
                        gnutls_strerror(err));
@@ -758,7 +764,9 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert,
  error:
     if (isServer)
         gnutls_dh_params_deinit(ctxt->dhParams);
-    gnutls_certificate_free_credentials(ctxt->x509cred);
+    if (ctxt->x509cred)
+        gnutls_certificate_free_credentials(ctxt->x509cred);
+    VIR_FREE(ctxt->priority);
     VIR_FREE(ctxt);
     return NULL;
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/2] rpc: cleanup in virNetTLSContextNew

Posted by Daniel P. Berrangé 9 weeks ago
On Mon, Apr 15, 2019 at 08:29:42PM +0200, Adrian Brzezinski wrote:
> Failed new gnutls context allocations in virNetTLSContextNew function
> results in double free and segfault. Occasional memory leaks may also
> occur.
> 
> Signed-off-by: Adrian Brzezinski <redhat@adrb.pl>
> ---
>  src/rpc/virnettlscontext.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 72e9ed9..7b5d578 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -707,6 +707,12 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert,
>  
>      err = gnutls_certificate_allocate_credentials(&ctxt->x509cred);
>      if (err) {
> +       /* gnutls_certificate_credentials_t is complex structure with multiple
> +        * internal memory allocatons that can go wrong, so make sure that
> +        * reference is NULL.
> +        */

I changd this comment to

        /* While gnutls_certificate_credentials_t will free any
         * partially allocated credentials struct, it does not
         * set the returned pointer back to NULL after it is
         * freed in an error path.
         */


and then pushed the fix


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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/2] news: cleanup in virNetTLSContextNew

Posted by Adrian Brzezinski 9 weeks ago
Signed-off-by: Adrian Brzezinski <redhat@adrb.pl>
---
 docs/news.xml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 86c7734..9338381 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -51,6 +51,16 @@
       </change>
     </section>
     <section title="Bug fixes">
+      <change>
+        <summary>
+          rpc: cleanup in virNetTLSContextNew
+        </summary>
+        <description>
+          Failed new gnutls context allocations in virNetTLSContextNew
+          function results in double free and segfault. Occasional memory
+          leaks may also occur.
+        </description>
+      </change>
     </section>
   </release>
   <release version="v5.2.0" date="2019-04-03">
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel P. Berrangé 9 weeks ago
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@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Adrian Brzezinski 9 weeks ago
On 15.04.2019 11:21, 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:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1699062
>>
>> Signed-off-by: Adrian Brzezinski <redhat@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);

Please read gdb session from bugzilla.
What really failed is tlist allocation in
gnutls_certificate_allocate_credentials, so ctxt->x509cred isn't NULL.

But I got your point.

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


-- 
Adrian Brzeziński
Technical Care Center
EO Networks S.A.

ul. Jagiellońska 78, 03-301 Warszawa
tel: (022) 532-15-30, fax: (022) 532-15-31

e-mail : adrian.brzezinski@eo.pl

NIP : 5272604418, REGON : 141905973
Sąd Rejonowy dla m.st.. Warszawy w Warszawie XII Wydział Gospodarczy
Krajowego Rejestru Sądowego,
KRS: 0000332547, Kapitał zakładowy i kapitał wpłacony : 205 937,90 złotych.

Ten dokument zawiera informacje poufne, które mogą być również objęte
tajemnicą handlową lub służbową. Jest on przeznaczony do wyłącznego
użytku adresata. Jeśli nie są Państwo jego adresatem lub jeśli
otrzymaliście Państwo ten dokument omyłkowo, to wszelkie
rozpowszechnianie, dystrybucja, reprodukcja, kopiowanie, publikacja lub
wykorzystanie tego dokumentu czy też zawartych w nim informacji jest
zabronione. Jeśli otrzymaliście Państwo tę wiadomość przez pomyłkę,
prosimy o bezzwłoczne skontaktowanie się z nami oraz usunięcie tej
wiadomości z Państwa komputera.

This message may contain confidential and/or privileged information and
is intended solely for the use of the individual or entity to whom is
addressed. If you are not the intended recipient, then any disclosure,
copying, distribution or any other action in reliance upon is expressly
prohibited and may be unlawful. In this case, please advise the sender
by replying and deleting this message.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Ján Tomko 9 weeks ago
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@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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list