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

[Erik Skultety]

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