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

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(a)redhat.com&gt; > > --- > > 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