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

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