[libvirt] [PATCH] virsh.c: avoid leak on OOM error path
No one really cares if we leak memory while dying, but who knows... freeing that buffer may let us go down more gracefully. FYI, the leak is triggered when virFileReadAll succeeds (it allocates "buffer"), yet xmlNewDoc fails: if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return FALSE; doc = xmlNewDoc(NULL); if (doc == NULL) goto no_memory;
From 03c7a44e3d5b2e7c992bebc98fc8c6a7bf63881e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 19 Feb 2010 18:03:41 +0100 Subject: [PATCH] virsh.c: avoid leak on OOM error path
* tools/virsh.c (cmdCPUBaseline): Also free "buffer" upon OOM. --- tools/virsh.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index dd916f3..8756a7a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7139,6 +7139,7 @@ cleanup: return ret; no_memory: + VIR_FREE(buffer); vshError(ctl, "%s", _("Out of memory")); ret = FALSE; return ret; -- 1.7.0.233.g05e1a
Jim Meyering wrote:
No one really cares if we leak memory while dying, but who knows... freeing that buffer may let us go down more gracefully.
FYI, the leak is triggered when virFileReadAll succeeds (it allocates "buffer"), yet xmlNewDoc fails:
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return FALSE;
doc = xmlNewDoc(NULL); if (doc == NULL) goto no_memory;
From 03c7a44e3d5b2e7c992bebc98fc8c6a7bf63881e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 19 Feb 2010 18:03:41 +0100 Subject: [PATCH] virsh.c: avoid leak on OOM error path
* tools/virsh.c (cmdCPUBaseline): Also free "buffer" upon OOM. --- tools/virsh.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index dd916f3..8756a7a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7139,6 +7139,7 @@ cleanup: return ret;
no_memory: + VIR_FREE(buffer); vshError(ctl, "%s", _("Out of memory")); ret = FALSE; return ret; --
The above is correct, but there's another leak in the same function, so I've amended the patch to also free the "list" buffer. "list" is allocated in the for-loop. If on the 2nd or subsequent iteration of that loop we take the "goto no_memory", we'd leak that buffer.
From 3d97412799c1b5bfedc647059c7d3b18e763f0bc Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 19 Feb 2010 18:03:41 +0100 Subject: [PATCH] virsh.c: avoid leak on OOM error path
* tools/virsh.c (cmdCPUBaseline): Also free "buffer" and "list" upon OOM. --- tools/virsh.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index dd916f3..c8ae9f2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7139,6 +7139,8 @@ cleanup: return ret; no_memory: + VIR_FREE(list); + VIR_FREE(buffer); vshError(ctl, "%s", _("Out of memory")); ret = FALSE; return ret; -- 1.7.0.233.g05e1a
No one really cares if we leak memory while dying, but who knows... freeing that buffer may let us go down more gracefully.
FYI, the leak is triggered when virFileReadAll succeeds (it allocates "buffer"), yet xmlNewDoc fails:
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return FALSE;
doc = xmlNewDoc(NULL); if (doc == NULL) goto no_memory;
The above is correct, but there's another leak in the same function, so I've amended the patch to also free the "list" buffer. "list" is allocated in the for-loop. If on the 2nd or subsequent iteration of that loop we take the "goto no_memory", we'd leak that buffer.
diff --git a/tools/virsh.c b/tools/virsh.c index dd916f3..c8ae9f2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7139,6 +7139,8 @@ cleanup: return ret;
no_memory: + VIR_FREE(list); + VIR_FREE(buffer); vshError(ctl, "%s", _("Out of memory")); ret = FALSE; return ret;
Actually that's not enough and it also duplicates code as all the cleanup code is already there: cleanup: xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); xmlFreeDoc(doc); VIR_FREE(result); if ((list != NULL) && (count > 0)) { for (i = 0;i < count;i++) VIR_FREE(list[i]); } VIR_FREE(list); VIR_FREE(buffer); return ret; The best solution is jumping to cleanup in OOM path. I should have spotted missing goto cleanup at the end of no_memory when I was reviewing the code... Patch attached... Jirka
Jiri Denemark wrote:
No one really cares if we leak memory while dying, but who knows... freeing that buffer may let us go down more gracefully.
FYI, the leak is triggered when virFileReadAll succeeds (it allocates "buffer"), yet xmlNewDoc fails:
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return FALSE;
doc = xmlNewDoc(NULL); if (doc == NULL) goto no_memory;
The above is correct, but there's another leak in the same function, so I've amended the patch to also free the "list" buffer. "list" is allocated in the for-loop. If on the 2nd or subsequent iteration of that loop we take the "goto no_memory", we'd leak that buffer.
diff --git a/tools/virsh.c b/tools/virsh.c index dd916f3..c8ae9f2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7139,6 +7139,8 @@ cleanup: return ret;
no_memory: + VIR_FREE(list); + VIR_FREE(buffer); vshError(ctl, "%s", _("Out of memory")); ret = FALSE; return ret;
Actually that's not enough and it also duplicates code as all the cleanup code is already there:
Indeed. The only piece of cleanup: code that is not needed on the current no_memory path is the freeing of "result". Plugging more leaks and avoiding duplication. Definite improvement. ACK.
diff --git a/tools/virsh.c b/tools/virsh.c index dd916f3..c8ae9f2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7139,6 +7139,8 @@ cleanup: return ret;
no_memory: + VIR_FREE(list); + VIR_FREE(buffer); vshError(ctl, "%s", _("Out of memory")); ret = FALSE; return ret;
Actually that's not enough and it also duplicates code as all the cleanup code is already there:
Indeed. The only piece of cleanup: code that is not needed on the current no_memory path is the freeing of "result". Plugging more leaks and avoiding duplication. Definite improvement. ACK.
Thanks, pushed.
On Fri, Feb 19, 2010 at 06:09:13PM +0100, Jim Meyering wrote:
No one really cares if we leak memory while dying, but who knows...
especially in virsh ...
freeing that buffer may let us go down more gracefully.
FYI, the leak is triggered when virFileReadAll succeeds (it allocates "buffer"), yet xmlNewDoc fails:
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return FALSE;
doc = xmlNewDoc(NULL); if (doc == NULL) goto no_memory;
From 03c7a44e3d5b2e7c992bebc98fc8c6a7bf63881e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 19 Feb 2010 18:03:41 +0100 Subject: [PATCH] virsh.c: avoid leak on OOM error path
* tools/virsh.c (cmdCPUBaseline): Also free "buffer" upon OOM. --- tools/virsh.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index dd916f3..8756a7a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7139,6 +7139,7 @@ cleanup: return ret;
no_memory: + VIR_FREE(buffer); vshError(ctl, "%s", _("Out of memory")); ret = FALSE; return ret;
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/
participants (3)
-
Daniel Veillard -
Jim Meyering -
Jiri Denemark