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

Re: [libvirt] [PATCHv2] snapshot: fix memory leak on error



On Thu, Apr 05, 2012 at 04:01:03PM -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 (VIR_JSON_TYPE_PROTECT): New marker.
> * src/util/json.c (virJSONValueFree): Use it to protect a portion
> of an array.
> (virJSONParserInsertValue, virJSONValueToStringOne): Sanity
> checking.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONTransaction): Avoid
> freeing caller's data.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive):
> Free actions array on failure.
> ---
> 
> v2: fix the problem correctly (v1 still had a latent leak in
> qemu_monitor.c:qemuMonitorTransaction() if it were ever called with
> a text monitor)
> 
> Okay, so v1 was minimal, and v2 is 18 times more lines of diff,
> but I like this version better, as it makes it so I don't have
> to think as hard in the future when I add more code that uses
> transaction.
> 
>  src/qemu/qemu_driver.c       |    1 +
>  src/qemu/qemu_monitor_json.c |   11 ++++++-----
>  src/util/json.c              |   17 +++++++++++++----
>  src/util/json.h              |    7 ++++---
>  4 files changed, 24 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..e1803bf 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;
> +    int type;
> 
>      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.  */
> +    type = actions->type;
> +    actions->type = VIR_JSON_TYPE_PROTECT;
>      virJSONValueFree(cmd);
> +    actions->type = type;
>      virJSONValueFree(reply);
>      return ret;
>  }
> diff --git a/src/util/json.c b/src/util/json.c
> index a85f580..2e1295a 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
> @@ -70,7 +70,7 @@ void virJSONValueFree(virJSONValuePtr value)
>      if (!value)
>          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,11 @@ 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;
> +    case VIR_JSON_TYPE_PROTECT:
> +        return;
>      }
> 
>      VIR_FREE(value);
> @@ -633,6 +638,10 @@ int virJSONValueObjectIsNull(virJSONValuePtr object, const char *key)
>  static int virJSONParserInsertValue(virJSONParserPtr parser,
>                                      virJSONValuePtr value)
>  {
> +    if (value->type == VIR_JSON_TYPE_PROTECT) {
> +        VIR_DEBUG("invalid value to insert");
> +        return -1;
> +    }
>      if (!parser->head) {
>          parser->head = value;
>      } else {
> @@ -968,7 +977,7 @@ static int virJSONValueToStringOne(virJSONValuePtr object,
> 
>      VIR_DEBUG("object=%p type=%d gen=%p", object, object->type, g);
> 
> -    switch (object->type) {
> +    switch ((virJSONType) object->type) {
>      case VIR_JSON_TYPE_OBJECT:
>          if (yajl_gen_map_open(g) != yajl_gen_status_ok)
>              return -1;
> @@ -1017,7 +1026,7 @@ static int virJSONValueToStringOne(virJSONValuePtr object,
>              return -1;
>          break;
> 
> -    default:
> +    case VIR_JSON_TYPE_PROTECT:
>          return -1;
>      }
> 
> diff --git a/src/util/json.h b/src/util/json.h
> index 4572654..88c9ab4 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,15 @@
>  # 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,
> -};
> +    VIR_JSON_TYPE_PROTECT,
> +} virJSONType;
> 
>  typedef struct _virJSONValue virJSONValue;
>  typedef virJSONValue *virJSONValuePtr;
> -- 
> 1.7.7.6

  Okay, though the 'PROTECT' really ought to be a flag on top of the
type of the virJSONType (after all it's really about allocation stategy
rather than a type information) but as a temporary measure, ACK, but
please squash a comment in for the new enum type,

Daniel

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


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