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@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