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 :|