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
> +++ b/daemon/dispatch.c
> @@ -112,8 +112,7 @@ void remoteDispatchOOMError (remote_error *rerr)
> }
>
>
> -void remoteDispatchConnError (remote_error *rerr,
> - virConnectPtr conn ATTRIBUTE_UNUSED)
> +void remoteDispatchError(remote_error *rerr)
Two cleanups for the price of one (rename function to nuke unused
parameter, while adding goto cleanup); but I'm okay with lumping them in
the same commit.
> @@ -458,20 +467,25 @@ remoteDispatchSupportsFeature(struct qemud_server *server ATTRIBUTE_UNUSED,
> remote_error *rerr,
> remote_supports_feature_args *args, remote_supports_feature_ret *ret)
> {
> + int rv = -1;
>
> if (!conn) {
> - remoteDispatchFormatError(rerr, "%s", _("connection not open"));
> - return -1;
> + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
Is virNetError really the best name for the new helper macro, especially
since VIR_FROM_THIS is VIR_FROM_REMOTE? Should it instead be named
virRemoteError()?
> @@ -500,11 +514,16 @@ remoteDispatchGetType(struct qemud_server *server ATTRIBUTE_UNUSED,
> */
> ret->type = strdup(type);
> if (!ret->type) {
> - remoteDispatchOOMError(rerr);
> - return -1;
> + virReportOOMError();
> + goto cleanup;
> }
>
> - return 0;
> + rv = 0;
> +
> +cleanup:
> + if (rv < 0)
> + remoteDispatchError(rerr);
That works. I wonder if we can completely get rid of
remoteDispatchOOMError?
> @@ -837,51 +911,47 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
> remote_domain_get_scheduler_parameters_args *args,
> remote_domain_get_scheduler_parameters_ret *ret)
> {
> if (VIR_ALLOC_N(params, nparams) < 0) {
> - remoteDispatchOOMError(rerr);
> - return -1;
> + virReportOOMError();
> + goto cleanup;
This could be goto no_memory, rather than open-coding the OOM call,
since you already have that label for other reasons in this function.
> @@ -1146,21 +1232,21 @@ remoteDispatchDomainBlockPeek(struct qemud_server *server ATTRIBUTE_UNUSED,
> remote_domain_block_peek_args *args,
> remote_domain_block_peek_ret *ret)
> {
> if (virDomainBlockPeek(dom, path, offset, size,
> ret->buffer.buffer_val, flags) == -1) {
> - /* free(ret->buffer.buffer_val); - caller frees */
> - remoteDispatchConnError(rerr, conn);
> - virDomainFree(dom);
> - return -1;
> + goto cleanup;
> }
> - virDomainFree(dom);
>
> - return 0;
> + rv = 0;
> +
> +cleanup:
> + if (rv < 0) {
> + remoteDispatchError(rerr);
> + VIR_FREE(ret->buffer.buffer_val);
Can we also clean up the caller to quit freeing (as a separate patch),
now that we guarantee the cleanup here? Should we have a 'make
syntax-check' rule that catches unadorned use of free() instead of VIR_FREE?
> @@ -1780,46 +1959,46 @@ remoteDispatchDomainGetSecurityLabel(struct qemud_server *server ATTRIBUTE_UNUSE
> remote_domain_get_security_label_args *args,
> remote_domain_get_security_label_ret *ret)
> {
> - virDomainPtr dom;
> + virDomainPtr dom = NULL;
> virSecurityLabelPtr seclabel;
> + int rv = -1;
>
> if (!conn) {
> - remoteDispatchFormatError(rerr, "%s", _("connection not open"));
> - return -1;
> + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
> +cleanup:
> + if (rv < 0)
> + remoteDispatchError(rerr);
> + if (dom)
> + virDomainFree(dom);
> + VIR_FREE(seclabel);
Oops - freeing an uninitialized pointer. Init seclabel to NULL.
> @@ -2730,37 +3039,37 @@ remoteDispatchDomainSetMemoryParameters(struct qemud_server *server
> remote_domain_set_memory_parameters_args
> * args, void *ret ATTRIBUTE_UNUSED)
> {
> - virDomainPtr dom;
> + virDomainPtr dom = NULL;
> int i, r, nparams;
> virMemoryParameterPtr params;
> unsigned int flags;
> + int rv = -1;
>
> if (!conn) {
> - remoteDispatchFormatError(rerr, "%s", _("connection not open"));
> - return -1;
> + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
> +cleanup:
> + if (rv < 0)
> + remoteDispatchError(rerr);
> + VIR_FREE(params);
Oops - another free(uninitialized).
> @@ -2830,41 +3141,37 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
> remote_domain_get_memory_parameters_ret
> * ret)
> {
> - virDomainPtr dom;
> + virDomainPtr dom = NULL;
> virMemoryParameterPtr params;
> int i, r, nparams;
> unsigned int flags;
> + int rv = -1;
>
> if (!conn) {
> - remoteDispatchFormatError(rerr, "%s", _("connection not open"));
> - return -1;
> + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
And again, params must be NULL.
> - success:
> - virDomainFree(dom);
> - VIR_FREE(params);
> -
> - return 0;
> +success:
> + rv = 0;
>
> - oom:
> - remoteDispatchOOMError(rerr);
> - cleanup:
> - virDomainFree(dom);
> - for (i = 0; i < nparams; i++)
> - VIR_FREE(ret->params.params_val[i].field);
> +no_memory:
> + virReportOOMError();
Ouch. You didn't mean for success: to fall through to no_memory:, but
to cleanup:. Sink the OOM reporting down, and then jump back to cleanup.
> @@ -2951,37 +3260,37 @@ remoteDispatchDomainSetBlkioParameters(struct qemud_server *server
> remote_domain_set_blkio_parameters_args
> * args, void *ret ATTRIBUTE_UNUSED)
> {
> - virDomainPtr dom;
> + virDomainPtr dom = NULL;
> int i, r, nparams;
> virBlkioParameterPtr params;
Same problem as SetMemoryParameters.
> @@ -3051,41 +3362,37 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server
> remote_domain_get_blkio_parameters_ret
> * ret)
> {
> - virDomainPtr dom;
> + virDomainPtr dom = NULL;
> virBlkioParameterPtr params;
> int i, r, nparams;
> unsigned int flags;
> +
> +no_memory:
> + virReportOOMError();
> + goto cleanup;
> }
Same problem as GetMemoryParameters with params being initialized, but
at least the no_memory label worked this time.
> @@ -4287,7 +4814,7 @@ remoteDispatchAuthList(struct qemud_server *server,
> static int
> remoteDispatchAuthSaslInit(struct qemud_server *server,
> struct qemud_client *client,
> - virConnectPtr conn,
> + virConnectPtr conn ATTRIBUTE_UNUSED,
> remote_message_header *hdr ATTRIBUTE_UNUSED,
> remote_error *rerr,
> void *args ATTRIBUTE_UNUSED,
> if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) {
> - remoteDispatchConnError(rerr, conn);
> goto error;
Losing the error reporting here...
> @@ -4439,7 +4964,7 @@ authfail:
> error:
> PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_SASL);
> virMutexUnlock(&client->lock);
> - return -1;
> + goto error;
...and replacing it with an infinite loop here. I don't think that's
what you meant to do.
> @@ -4600,7 +5125,7 @@ remoteDispatchAuthSaslStart(struct qemud_server *server,
> /* NB, distinction of NULL vs "" is *critical* in SASL */
> if (serverout) {
> if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) {
> - remoteDispatchOOMError(rerr);
> + virReportOOMError();
> goto error;
> }
This changes from remoteDispatchOOMError to virReportOOM, but the error:
label doesn't call remoteDispatchError to actually dispatch it.
> @@ -4701,7 +5226,7 @@ remoteDispatchAuthSaslStep(struct qemud_server *server,
> /* NB, distinction of NULL vs "" is *critical* in SASL */
> if (serverout) {
> if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) {
> - remoteDispatchOOMError(rerr);
> + virReportOOMError();
> goto error;
> }
Likewise.
> @@ -4878,7 +5403,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server,
> client->auth = REMOTE_AUTH_NONE;
>
> virMutexUnlock(&client->lock);
> - return 0;
> + rv = 0;
>
> authfail:
> PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd,
REMOTE_AUTH_POLKIT);
Oops - that was unintended fallthrough.
> @@ -5010,7 +5535,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server,
> client->auth = REMOTE_AUTH_NONE;
>
> virMutexUnlock(&client->lock);
> - return 0;
> + rv = 0;
>
> authfail:
> PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_POLKIT);
As was that.
> @@ -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.
> @@ -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.
> @@ -7784,32 +8672,36 @@ remoteDispatchDomainSnapshotCurrent(struct qemud_server *server ATTRIBUTE_UNUSED
> remote_domain_snapshot_current_ret *ret)
> {
> virDomainSnapshotPtr snapshot;
> - virDomainPtr domain;
> + virDomainPtr domain = NULL;
> + int rv = -1;
>
> if (!conn) {
> - remoteDispatchFormatError(rerr, "%s", _("connection not open"));
> - return -1;
> + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
> +cleanup:
> + if (rv < 0)
> + remoteDispatchError(rerr);
> + if (snapshot)
> + virDomainSnapshotFree(snapshot);
Oops, freeing uninitialized variable.
All the rest look fine.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list