[libvirt] [PATCHv3] snapshot: fix memory leak on error

[Daniel Veillard]

On Thu, Apr 05, 2012 at 10:24:49PM -0600, Eric Blake wrote:
> Leak introduced in commit 0436d32.  If we allocate an actions array,
> but fail early enough to never consume it with the qemu monitor
> transaction call, we leaked memory.
> 
> But our semantics of making the transaction command free the caller's
> memory is awkward; avoiding the memory leak requires making every
> intermediate function in the call chain check for error.  It is much
> easier to fix things so that the function that allocates also frees,
> while the call chain leaves the caller's data intact.  To do that,
> I had to hack our JSON data structure to make it easy to protect a
> portion of an arbitrary JSON tree from being freed.
> 
> * src/util/json.h (virJSONType): Name the enum.
> (_virJSONValue): New field.
> * src/util/json.c (virJSONValueFree): Use it to protect a portion
> of an array.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONTransaction): Avoid
> freeing caller's data.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive):
> Free actions array on failure.
> ---
> 
> v3: make the protection orthogonal to the typing.
> 
>  src/qemu/qemu_driver.c       |    1 +
>  src/qemu/qemu_monitor_json.c |   11 ++++++-----
>  src/util/json.c              |    9 ++++++---
>  src/util/json.h              |    9 +++++----
>  4 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index dd79973..0456b34 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10127,6 +10127,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
>      if (actions) {
>          if (ret == 0)
>              ret = qemuMonitorTransaction(priv->mon, actions);
> +        virJSONValueFree(actions);
>          if (ret < 0) {
>              /* Transaction failed; undo the changes to vm.  */
>              bool need_unlink = !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 7093e1d..8eeb307 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3157,22 +3157,19 @@ cleanup:
>      return ret;
>  }
> 
> -/* Note that this call frees actions regardless of whether the call
> - * succeeds.  */
>  int
>  qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
>  {
>      int ret;
>      virJSONValuePtr cmd;
>      virJSONValuePtr reply = NULL;
> +    bool protect;
> 
>      cmd = qemuMonitorJSONMakeCommand("transaction",
>                                       "a:actions", actions,
>                                       NULL);
> -    if (!cmd) {
> -        virJSONValueFree(actions);
> +    if (!cmd)
>          return -1;
> -    }
> 
>      if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
>          goto cleanup;
> @@ -3180,7 +3177,11 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
>      ret = qemuMonitorJSONCheckError(cmd, reply);
> 
>  cleanup:
> +    /* We do NOT want to free actions when recursively freeing cmd.  */
> +    protect = actions->protect;
> +    actions->protect = true;
>      virJSONValueFree(cmd);
> +    actions->protect = protect;
>      virJSONValueFree(reply);
>      return ret;
>  }
> diff --git a/src/util/json.c b/src/util/json.c
> index a85f580..3258c3f 100644
> --- a/src/util/json.c
> +++ b/src/util/json.c
> @@ -1,7 +1,7 @@
>  /*
>   * json.c: JSON object parsing/formatting
>   *
> - * Copyright (C) 2009-2010 Red Hat, Inc.
> + * Copyright (C) 2009-2010, 2012 Red Hat, Inc.
>   * Copyright (C) 2009 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -67,10 +67,10 @@ struct _virJSONParser {
>  void virJSONValueFree(virJSONValuePtr value)
>  {
>      int i;
> -    if (!value)
> +    if (!value || value->protect)
>          return;
> 
> -    switch (value->type) {
> +    switch ((virJSONType) value->type) {
>      case VIR_JSON_TYPE_OBJECT:
>          for (i = 0 ; i < value->data.object.npairs; i++) {
>              VIR_FREE(value->data.object.pairs[i].key);
> @@ -89,6 +89,9 @@ void virJSONValueFree(virJSONValuePtr value)
>      case VIR_JSON_TYPE_NUMBER:
>          VIR_FREE(value->data.number);
>          break;
> +    case VIR_JSON_TYPE_BOOLEAN:
> +    case VIR_JSON_TYPE_NULL:
> +        break;
>      }
> 
>      VIR_FREE(value);
> diff --git a/src/util/json.h b/src/util/json.h
> index 4572654..686a8fb 100644
> --- a/src/util/json.h
> +++ b/src/util/json.h
> @@ -1,8 +1,8 @@
>  /*
>   * json.h: JSON object parsing/formatting
>   *
> + * Copyright (C) 2009, 2012 Red Hat, Inc.
>   * Copyright (C) 2009 Daniel P. Berrange
> - * Copyright (C) 2009 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -27,14 +27,14 @@
>  # include "internal.h"
> 
> 
> -enum {
> +typedef enum {
>      VIR_JSON_TYPE_OBJECT,
>      VIR_JSON_TYPE_ARRAY,
>      VIR_JSON_TYPE_STRING,
>      VIR_JSON_TYPE_NUMBER,
>      VIR_JSON_TYPE_BOOLEAN,
>      VIR_JSON_TYPE_NULL,
> -};
> +} virJSONType;
> 
>  typedef struct _virJSONValue virJSONValue;
>  typedef virJSONValue *virJSONValuePtr;
> @@ -65,7 +65,8 @@ struct _virJSONArray {
>  };
> 
>  struct _virJSONValue {
> -    int type;
> +    int type; /* enum virJSONType */
> +    bool protect; /* prevents deletion when embedded in another object */
> 
>      union {
>          virJSONObject object;

  ACK, sounds better to me than piggy-backing on an enum value :-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/