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