[libvirt] [PATCH] snapshot: fix memory leak on error
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. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Free actions array on failure. --- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b29029e..a214593 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10132,6 +10132,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, if (actions) { if (ret == 0) ret = qemuMonitorTransaction(priv->mon, actions); + else + virJSONValueFree(actions); if (ret < 0) { /* Transaction failed; undo the changes to vm. */ bool need_unlink = !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT); -- 1.7.7.6
On 04/05/2012 03:16 PM, 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.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Free actions array on failure. --- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b29029e..a214593 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10132,6 +10132,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, if (actions) { if (ret == 0) ret = qemuMonitorTransaction(priv->mon, actions); + else + virJSONValueFree(actions); if (ret < 0) { /* Transaction failed; undo the changes to vm. */ bool need_unlink = !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT);
ACK.
On 04/05/2012 01:34 PM, Laine Stump wrote:
On 04/05/2012 03:16 PM, 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.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Free actions array on failure. --- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b29029e..a214593 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10132,6 +10132,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, if (actions) { if (ret == 0) ret = qemuMonitorTransaction(priv->mon, actions); + else + virJSONValueFree(actions); if (ret < 0) { /* Transaction failed; undo the changes to vm. */ bool need_unlink = !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT);
ACK.
Shoot - I just realized that I missed a spot. The semantics of consuming an array are a bit confusing, since it means everyone along the chain must participate in those semantics. Better is to make the person that allocates also do the free, but that requires a bit more hacking to avoid double-freeing things when recursively deleting the 'transaction' JSON command that wraps the actions array. v2 coming up. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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/
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; -- 1.7.7.6
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 04/05/2012 10:56 PM, Daniel Veillard wrote:
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.
ACK, sounds better to me than piggy-backing on an enum value :-)
Thanks. But I found one more (unlikely) issue: qemuMonitorJSONMakeCommand can also trigger a recursive free on a partial construction when hitting OOM. So I'm pushing with this additional modification: diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c index 8eeb307..d09d779 100644 --- i/src/qemu/qemu_monitor_json.c +++ w/src/qemu/qemu_monitor_json.c @@ -3160,16 +3160,18 @@ cleanup: int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) { - int ret; + int ret = -1; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; - bool protect; + bool protect = actions->protect; + /* We do NOT want to free actions when recursively freeing cmd. */ + actions->protect = true; cmd = qemuMonitorJSONMakeCommand("transaction", "a:actions", actions, NULL); if (!cmd) - return -1; + goto cleanup; if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) goto cleanup; @@ -3177,12 +3179,9 @@ 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); + actions->protect = protect; return ret; } -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
gcc 4.7 warns about uninitialized struct members * tests/testutilsqemu.c (testQemuCapsInit): Populate new members. * tests/viruritest.c (mymain): Likewise. --- Another warning cleanup, worth pushing at the same time as whichever solution we pick for the large stack size. tests/testutilsqemu.c | 6 +++++- tests/viruritest.c | 34 +++++++++++++++++----------------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8e621fe..8d5a3bf 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -123,7 +123,11 @@ virCapsPtr testQemuCapsInit(void) { 1, /* threads */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */ - host_cpu_features /* features */ + host_cpu_features, /* features */ + 0, /* ncells */ + 0, /* ncells_max */ + NULL, /* cells */ + 0, /* cells_cpus */ }; if ((caps = virCapabilitiesNew(host_cpu.arch, diff --git a/tests/viruritest.c b/tests/viruritest.c index 3570217..c0ab9a1 100644 --- a/tests/viruritest.c +++ b/tests/viruritest.c @@ -160,8 +160,8 @@ mymain(void) "test", "example.com", 0, "/", query_in, NULL, params) virURIParam params[] = { - { (char*)"name", (char*)"value" }, - { NULL, NULL }, + { (char*)"name", (char*)"value", false }, + { NULL, NULL, false }, }; TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL, NULL); @@ -172,31 +172,31 @@ mymain(void) TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL, NULL); virURIParam params1[] = { - { (char*)"foo", (char*)"one" }, - { (char*)"bar", (char*)"two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one", false }, + { (char*)"bar", (char*)"two", false }, + { NULL, NULL, false }, }; virURIParam params2[] = { - { (char*)"foo", (char*)"one" }, - { (char*)"foo", (char*)"two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one", false }, + { (char*)"foo", (char*)"two", false }, + { NULL, NULL, false }, }; virURIParam params3[] = { - { (char*)"foo", (char*)"&one" }, - { (char*)"bar", (char*)"&two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"&one", false }, + { (char*)"bar", (char*)"&two", false }, + { NULL, NULL, false }, }; virURIParam params4[] = { - { (char*)"foo", (char*)"" }, - { NULL, NULL }, + { (char*)"foo", (char*)"", false }, + { NULL, NULL, false }, }; virURIParam params5[] = { - { (char*)"foo", (char*)"one two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one two", false }, + { NULL, NULL, false }, }; virURIParam params6[] = { - { (char*)"foo", (char*)"one" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one", false }, + { NULL, NULL, false }, }; TEST_PARAMS("foo=one&bar=two", "", params1); -- 1.7.7.6
On 04/05/2012 08:22 PM, Eric Blake wrote:
gcc 4.7 warns about uninitialized struct members
* tests/testutilsqemu.c (testQemuCapsInit): Populate new members. * tests/viruritest.c (mymain): Likewise. ---
Another warning cleanup, worth pushing at the same time as whichever solution we pick for the large stack size.
I threaded this one wrong; it was intended to be a reply to https://www.redhat.com/archives/libvir-list/2012-April/msg00209.html -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Apr 05, 2012 at 08:22:51PM -0600, Eric Blake wrote:
gcc 4.7 warns about uninitialized struct members
* tests/testutilsqemu.c (testQemuCapsInit): Populate new members. * tests/viruritest.c (mymain): Likewise. ---
Another warning cleanup, worth pushing at the same time as whichever solution we pick for the large stack size.
tests/testutilsqemu.c | 6 +++++- tests/viruritest.c | 34 +++++++++++++++++----------------- 2 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8e621fe..8d5a3bf 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -123,7 +123,11 @@ virCapsPtr testQemuCapsInit(void) { 1, /* threads */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */ - host_cpu_features /* features */ + host_cpu_features, /* features */ + 0, /* ncells */ + 0, /* ncells_max */ + NULL, /* cells */ + 0, /* cells_cpus */ };
if ((caps = virCapabilitiesNew(host_cpu.arch, diff --git a/tests/viruritest.c b/tests/viruritest.c index 3570217..c0ab9a1 100644 --- a/tests/viruritest.c +++ b/tests/viruritest.c @@ -160,8 +160,8 @@ mymain(void) "test", "example.com", 0, "/", query_in, NULL, params)
virURIParam params[] = { - { (char*)"name", (char*)"value" }, - { NULL, NULL }, + { (char*)"name", (char*)"value", false }, + { NULL, NULL, false }, };
TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL, NULL); @@ -172,31 +172,31 @@ mymain(void) TEST_PARSE("test://[2001:41c8:1:4fd4::2]:123/system", "test", "2001:41c8:1:4fd4::2", 123, "/system", NULL, NULL, NULL);
virURIParam params1[] = { - { (char*)"foo", (char*)"one" }, - { (char*)"bar", (char*)"two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one", false }, + { (char*)"bar", (char*)"two", false }, + { NULL, NULL, false }, }; virURIParam params2[] = { - { (char*)"foo", (char*)"one" }, - { (char*)"foo", (char*)"two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one", false }, + { (char*)"foo", (char*)"two", false }, + { NULL, NULL, false }, }; virURIParam params3[] = { - { (char*)"foo", (char*)"&one" }, - { (char*)"bar", (char*)"&two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"&one", false }, + { (char*)"bar", (char*)"&two", false }, + { NULL, NULL, false }, }; virURIParam params4[] = { - { (char*)"foo", (char*)"" }, - { NULL, NULL }, + { (char*)"foo", (char*)"", false }, + { NULL, NULL, false }, }; virURIParam params5[] = { - { (char*)"foo", (char*)"one two" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one two", false }, + { NULL, NULL, false }, }; virURIParam params6[] = { - { (char*)"foo", (char*)"one" }, - { NULL, NULL }, + { (char*)"foo", (char*)"one", false }, + { NULL, NULL, false }, };
TEST_PARAMS("foo=one&bar=two", "", params1);
ACK, 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/
On 04/05/2012 09:42 PM, Daniel Veillard wrote:
On Thu, Apr 05, 2012 at 08:22:51PM -0600, Eric Blake wrote:
gcc 4.7 warns about uninitialized struct members
* tests/testutilsqemu.c (testQemuCapsInit): Populate new members. * tests/viruritest.c (mymain): Likewise. ---
Another warning cleanup, worth pushing at the same time as whichever solution we pick for the large stack size.
ACK,
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Laine Stump