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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list