On Wed, Apr 13, 2011 at 05:23:41PM -0600, Eric Blake wrote:
> On 04/13/2011 12:12 PM, Daniel P. Berrange wrote:
> > The dispatcher functions have numerous places where they
> > return to the caller. This leads to duplicated cleanup
> > code, often resulting in memory leaks. It makes it harder
> > to ensure that errors are dispatched before freeing objects,
> > which may overwrite the original error.
> >
>
> > * daemon/remote.c: Centralize all cleanup paths
> > * daemon/stream.c: s/remoteDispatchConnError/remoteDispatchError/
> > * daemon/dispatch.c, daemon/dispatch.h: Replace
> > remoteDispatchConnError with remoteDispatchError
> > removing unused virConnectPtr
> > @@ -6245,18 +6963,18 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED,
> > remote_node_device_get_parent_args *args,
> > remote_node_device_get_parent_ret *ret)
> > {
> > - virNodeDevicePtr dev;
> > + virNodeDevicePtr dev = NULL;
> > const char *parent;
> > + int rv = -1;
> >
> > if (!conn) {
> > - remoteDispatchFormatError(rerr, "%s", _("connection not open"));
> > - return -1;
> > + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> > + goto cleanup;
>
> Oops, parent is uninitialized on this path...
>
> > }
> >
> > dev = virNodeDeviceLookupByName(conn, args->name);
> > if (dev == NULL) {
> > - remoteDispatchConnError(rerr, conn);
> > - return -1;
> > + goto cleanup;
> > }
> >
> > parent = virNodeDeviceGetParent(dev);
>
> ...and malloc'd on this path.
It isn't malloc'd here actually. This is returning
a 'const char *'...
>
> > @@ -6267,21 +6985,25 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED,
> > /* remoteDispatchClientRequest will free this. */
> > char **parent_p;
> > if (VIR_ALLOC(parent_p) < 0) {
> > - virNodeDeviceFree(dev);
> > - remoteDispatchOOMError(rerr);
> > - return -1;
> > + virReportOOMError();
> > + goto cleanup;
> > }
> > *parent_p = strdup(parent);
>
> Rather than strdup()ing malloc's memory, this should be:
>
> *parent_p = parent;
> parent = NULL;
>
> > if (*parent_p == NULL) {
> > - virNodeDeviceFree(dev);
> > - remoteDispatchOOMError(rerr);
> > - return -1;
> > + virReportOOMError();
> > + goto cleanup;
> > }
> > ret->parent = parent_p;
> > }
> >
> > - virNodeDeviceFree(dev);
> > - return 0;
> > + rv = 0;
> > +
> > +cleanup:
> > + if (rv < 0)
> > + remoteDispatchError(rerr);
> > + if (dev)
> > + virNodeDeviceFree(dev);
>
> ...and add unconditional VIR_FREE(parent) here. I'm surprised we
> haven't noticed that leak before.
..meaning it isn't actually a leak here :-)
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list