1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 00/12] Rework sysfs handling
  4. View patch

[libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities

Martin Kletzander posted 12 patches 8 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities
Posted by Martin Kletzander 8 years, 10 months ago 
If formatting NUMA topology fails, the function returns immediatelly,
but the buffer structure allocated on the stack references lot of
heap-allocated memory and that would get lost in such case.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/conf/capabilities.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 08907aced1b9..be95c50cfb67 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -955,7 +955,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
     if (caps->host.nnumaCell &&
         virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell,
                                           caps->host.numaCell) < 0)
-        return NULL;
+        goto error;

     for (i = 0; i < caps->host.nsecModels; i++) {
         virBufferAddLit(&buf, "<secmodel>\n");
@@ -1072,6 +1072,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
         return NULL;

     return virBufferContentAndReset(&buf);
+
+ error:
+    virBufferFreeAndReset(&buf);
+    return NULL;
 }

 /* get the maximum ID of cpus in the host */
-- 
2.12.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities
Posted by Erik Skultety 8 years, 10 months ago 
On Wed, Apr 05, 2017 at 04:36:24PM +0200, Martin Kletzander wrote:
> If formatting NUMA topology fails, the function returns immediatelly,
> but the buffer structure allocated on the stack references lot of
> heap-allocated memory and that would get lost in such case.
>
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/conf/capabilities.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 08907aced1b9..be95c50cfb67 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -955,7 +955,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>      if (caps->host.nnumaCell &&
>          virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell,
>                                            caps->host.numaCell) < 0)
> -        return NULL;
> +        goto error;

Personally, I'd more like cleanup, but looking at other XML formatting methods,
I'm fine with this as well, at least we stay consistent.

>
>      for (i = 0; i < caps->host.nsecModels; i++) {
>          virBufferAddLit(&buf, "<secmodel>\n");
> @@ -1072,6 +1072,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>          return NULL;
>

Git discarded the context here, so squash this in:

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index be95c50cf..ea6d4b19d 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -1069,7 +1069,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
     virBufferAddLit(&buf, "</capabilities>\n");

     if (virBufferCheckError(&buf) < 0)
-        return NULL;
+        goto error;

     return virBufferContentAndReset(&buf);


>      return virBufferContentAndReset(&buf);
> +
> + error:
> +    virBufferFreeAndReset(&buf);
> +    return NULL;
>  }
>
>  /* get the maximum ID of cpus in the host */
> --
> 2.12.2
>

ACK with the bit above squashed in.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities
Posted by Peter Krempa 8 years, 10 months ago 
On Thu, Apr 06, 2017 at 09:50:49 +0200, Erik Skultety wrote:
> On Wed, Apr 05, 2017 at 04:36:24PM +0200, Martin Kletzander wrote:
> > If formatting NUMA topology fails, the function returns immediatelly,
> > but the buffer structure allocated on the stack references lot of
> > heap-allocated memory and that would get lost in such case.
> >
> > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> > ---
> >  src/conf/capabilities.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > index 08907aced1b9..be95c50cfb67 100644
> > --- a/src/conf/capabilities.c
> > +++ b/src/conf/capabilities.c
> > @@ -955,7 +955,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> >      if (caps->host.nnumaCell &&
> >          virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell,
> >                                            caps->host.numaCell) < 0)
> > -        return NULL;
> > +        goto error;
> 
> Personally, I'd more like cleanup, but looking at other XML formatting methods,
> I'm fine with this as well, at least we stay consistent.

"cleanup" label should be used if the success path passes through the
label as well. If only the error path passes through it it should be
called "error".
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities
Posted by Martin Kletzander 8 years, 10 months ago 
On Thu, Apr 06, 2017 at 09:50:49AM +0200, Erik Skultety wrote:
>On Wed, Apr 05, 2017 at 04:36:24PM +0200, Martin Kletzander wrote:
>> If formatting NUMA topology fails, the function returns immediatelly,
>> but the buffer structure allocated on the stack references lot of
>> heap-allocated memory and that would get lost in such case.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/conf/capabilities.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> index 08907aced1b9..be95c50cfb67 100644
>> --- a/src/conf/capabilities.c
>> +++ b/src/conf/capabilities.c
>> @@ -955,7 +955,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>>      if (caps->host.nnumaCell &&
>>          virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell,
>>                                            caps->host.numaCell) < 0)
>> -        return NULL;
>> +        goto error;
>
>Personally, I'd more like cleanup, but looking at other XML formatting methods,
>I'm fine with this as well, at least we stay consistent.
>
>>
>>      for (i = 0; i < caps->host.nsecModels; i++) {
>>          virBufferAddLit(&buf, "<secmodel>\n");
>> @@ -1072,6 +1072,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>>          return NULL;
>>
>
>Git discarded the context here, so squash this in:
>
>diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>index be95c50cf..ea6d4b19d 100644
>--- a/src/conf/capabilities.c
>+++ b/src/conf/capabilities.c
>@@ -1069,7 +1069,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>     virBufferAddLit(&buf, "</capabilities>\n");
>
>     if (virBufferCheckError(&buf) < 0)
>-        return NULL;
>+        goto error;
>
>     return virBufferContentAndReset(&buf);
>

I don't really understand why would I need to do that.

>
>>      return virBufferContentAndReset(&buf);
>> +
>> + error:
>> +    virBufferFreeAndReset(&buf);
>> +    return NULL;
>>  }
>>
>>  /* get the maximum ID of cpus in the host */
>> --
>> 2.12.2
>>
>
>ACK with the bit above squashed in.
>
>Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities
Posted by Erik Skultety 8 years, 10 months ago 
On Thu, Apr 06, 2017 at 11:12:54AM +0200, Martin Kletzander wrote:
> On Thu, Apr 06, 2017 at 09:50:49AM +0200, Erik Skultety wrote:
> > On Wed, Apr 05, 2017 at 04:36:24PM +0200, Martin Kletzander wrote:
> > > If formatting NUMA topology fails, the function returns immediatelly,
> > > but the buffer structure allocated on the stack references lot of
> > > heap-allocated memory and that would get lost in such case.
> > >
> > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> > > ---
> > >  src/conf/capabilities.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > > index 08907aced1b9..be95c50cfb67 100644
> > > --- a/src/conf/capabilities.c
> > > +++ b/src/conf/capabilities.c
> > > @@ -955,7 +955,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> > >      if (caps->host.nnumaCell &&
> > >          virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell,
> > >                                            caps->host.numaCell) < 0)
> > > -        return NULL;
> > > +        goto error;
> >
> > Personally, I'd more like cleanup, but looking at other XML formatting methods,
> > I'm fine with this as well, at least we stay consistent.
> >
> > >
> > >      for (i = 0; i < caps->host.nsecModels; i++) {
> > >          virBufferAddLit(&buf, "<secmodel>\n");
> > > @@ -1072,6 +1072,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> > >          return NULL;
> > >
> >
> > Git discarded the context here, so squash this in:
> >
> > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > index be95c50cf..ea6d4b19d 100644
> > --- a/src/conf/capabilities.c
> > +++ b/src/conf/capabilities.c
> > @@ -1069,7 +1069,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> >     virBufferAddLit(&buf, "</capabilities>\n");
> >
> >     if (virBufferCheckError(&buf) < 0)
> > -        return NULL;
> > +        goto error;
> >
> >     return virBufferContentAndReset(&buf);
> >
>
> I don't really understand why would I need to do that.

Because buf->content might be non-NULL.

>
> >
> > >      return virBufferContentAndReset(&buf);
> > > +
> > > + error:
> > > +    virBufferFreeAndReset(&buf);
> > > +    return NULL;
> > >  }
> > >
> > >  /* get the maximum ID of cpus in the host */
> > > --
> > > 2.12.2
> > >
> >
> > ACK with the bit above squashed in.
> >
> > Erik


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities
Posted by Martin Kletzander 8 years, 10 months ago 
On Thu, Apr 06, 2017 at 11:29:21AM +0200, Erik Skultety wrote:
>On Thu, Apr 06, 2017 at 11:12:54AM +0200, Martin Kletzander wrote:
>> On Thu, Apr 06, 2017 at 09:50:49AM +0200, Erik Skultety wrote:
>> > On Wed, Apr 05, 2017 at 04:36:24PM +0200, Martin Kletzander wrote:
>> > > If formatting NUMA topology fails, the function returns immediatelly,
>> > > but the buffer structure allocated on the stack references lot of
>> > > heap-allocated memory and that would get lost in such case.
>> > >
>> > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> > > ---
>> > >  src/conf/capabilities.c | 6 +++++-
>> > >  1 file changed, 5 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> > > index 08907aced1b9..be95c50cfb67 100644
>> > > --- a/src/conf/capabilities.c
>> > > +++ b/src/conf/capabilities.c
>> > > @@ -955,7 +955,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>> > >      if (caps->host.nnumaCell &&
>> > >          virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell,
>> > >                                            caps->host.numaCell) < 0)
>> > > -        return NULL;
>> > > +        goto error;
>> >
>> > Personally, I'd more like cleanup, but looking at other XML formatting methods,
>> > I'm fine with this as well, at least we stay consistent.
>> >
>> > >
>> > >      for (i = 0; i < caps->host.nsecModels; i++) {
>> > >          virBufferAddLit(&buf, "<secmodel>\n");
>> > > @@ -1072,6 +1072,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>> > >          return NULL;
>> > >
>> >
>> > Git discarded the context here, so squash this in:
>> >
>> > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> > index be95c50cf..ea6d4b19d 100644
>> > --- a/src/conf/capabilities.c
>> > +++ b/src/conf/capabilities.c
>> > @@ -1069,7 +1069,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>> >     virBufferAddLit(&buf, "</capabilities>\n");
>> >
>> >     if (virBufferCheckError(&buf) < 0)
>> > -        return NULL;
>> > +        goto error;
>> >
>> >     return virBufferContentAndReset(&buf);
>> >
>>
>> I don't really understand why would I need to do that.
>
>Because buf->content might be non-NULL.
>

Buffers are automatically reset on errors as there is no need for the
data.  It also makes the function epilogues and error handling easier
and shorter (e.g. this case).  See virBufferSetError() for more info.

>>
>> >
>> > >      return virBufferContentAndReset(&buf);
>> > > +
>> > > + error:
>> > > +    virBufferFreeAndReset(&buf);
>> > > +    return NULL;
>> > >  }
>> > >
>> > >  /* get the maximum ID of cpus in the host */
>> > > --
>> > > 2.12.2
>> > >
>> >
>> > ACK with the bit above squashed in.
>> >
>> > Erik
>
>
>--
>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
Re: [libvirt] [PATCH v2 01/12] conf: Fix possible memleak in capabilities
Posted by Erik Skultety 8 years, 10 months ago 
On Thu, Apr 06, 2017 at 11:52:54AM +0200, Martin Kletzander wrote:
> On Thu, Apr 06, 2017 at 11:29:21AM +0200, Erik Skultety wrote:
> > On Thu, Apr 06, 2017 at 11:12:54AM +0200, Martin Kletzander wrote:
> > > On Thu, Apr 06, 2017 at 09:50:49AM +0200, Erik Skultety wrote:
> > > > On Wed, Apr 05, 2017 at 04:36:24PM +0200, Martin Kletzander wrote:
> > > > > If formatting NUMA topology fails, the function returns immediatelly,
> > > > > but the buffer structure allocated on the stack references lot of
> > > > > heap-allocated memory and that would get lost in such case.
> > > > >
> > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> > > > > ---
> > > > >  src/conf/capabilities.c | 6 +++++-
> > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > > > > index 08907aced1b9..be95c50cfb67 100644
> > > > > --- a/src/conf/capabilities.c
> > > > > +++ b/src/conf/capabilities.c
> > > > > @@ -955,7 +955,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> > > > >      if (caps->host.nnumaCell &&
> > > > >          virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell,
> > > > >                                            caps->host.numaCell) < 0)
> > > > > -        return NULL;
> > > > > +        goto error;
> > > >
> > > > Personally, I'd more like cleanup, but looking at other XML formatting methods,
> > > > I'm fine with this as well, at least we stay consistent.
> > > >
> > > > >
> > > > >      for (i = 0; i < caps->host.nsecModels; i++) {
> > > > >          virBufferAddLit(&buf, "<secmodel>\n");
> > > > > @@ -1072,6 +1072,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> > > > >          return NULL;
> > > > >
> > > >
> > > > Git discarded the context here, so squash this in:
> > > >
> > > > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> > > > index be95c50cf..ea6d4b19d 100644
> > > > --- a/src/conf/capabilities.c
> > > > +++ b/src/conf/capabilities.c
> > > > @@ -1069,7 +1069,7 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> > > >     virBufferAddLit(&buf, "</capabilities>\n");
> > > >
> > > >     if (virBufferCheckError(&buf) < 0)
> > > > -        return NULL;
> > > > +        goto error;
> > > >
> > > >     return virBufferContentAndReset(&buf);
> > > >
> > >
> > > I don't really understand why would I need to do that.
> >
> > Because buf->content might be non-NULL.
> >
>
> Buffers are automatically reset on errors as there is no need for the
> data.  It also makes the function epilogues and error handling easier
> and shorter (e.g. this case).  See virBufferSetError() for more info.

Fair enough, I missed the VIR_FREE at the top of the function when I was looking
at it.

>
> > >
> > > >
> > > > >      return virBufferContentAndReset(&buf);
> > > > > +
> > > > > + error:
> > > > > +    virBufferFreeAndReset(&buf);
> > > > > +    return NULL;
> > > > >  }
> > > > >
> > > > >  /* get the maximum ID of cpus in the host */
> > > > > --
> > > > > 2.12.2
> > > > >
> > > >
> > > > ACK with the bit above squashed in.
> > > >
> > > > Erik
> >
> >
> > --
> > 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.