[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Merge all returns paths from dispatcher into single path



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]