6:06 p.m.
# HG changeset patch
# User Kaitlin Rupert <karupert(a)us.ibm.com>
# Date 1194912941 28800
# Node ID f2966070990024afbad3ee79dd5f276b0310aa2e
# Parent 4ceb57b4430b0455ef7cb68f57a8ec6ad01abeca
This fixes a potential crash if _get_domain() is unable to properly parse the dom XML.
Also fixes an issue in VSSD - if get_domaininfo() returns 0 (an error), then
instance_from_dom() needs to set ret properly so that an empty instance won't be
created.
Signed-off-by: Kaitlin Rupert <karupert(a)us.ibm.com>
diff -r 4ceb57b4430b -r f29660709900 libxkutil/device_parsing.c
--- a/libxkutil/device_parsing.c Mon Nov 12 13:54:53 2007 -0800
+++ b/libxkutil/device_parsing.c Mon Nov 12 16:15:41 2007 -0800
@@ -575,10 +575,17 @@ int get_dominfo(virDomainPtr dom, struct
xml = virDomainGetXMLDesc(dom, 0);
if (!xml) {
free(*dominfo);
+ *dominfo = NULL;
return 0;
}
ret = _get_dominfo(xml, *dominfo);
+ if (!ret) {
+ free(*dominfo);
+ *dominfo = NULL;
+ return 0;
+ }
+
free(xml);
(*dominfo)->dev_mem_ct = get_mem_devices(dom, &(*dominfo)->dev_mem);
diff -r 4ceb57b4430b -r f29660709900 src/Virt_VSSD.c
--- a/src/Virt_VSSD.c Mon Nov 12 13:54:53 2007 -0800
+++ b/src/Virt_VSSD.c Mon Nov 12 16:15:41 2007 -0800
@@ -45,7 +45,8 @@ static int instance_from_dom(virDomainPt
CMPIObjectPath *op;
struct domain *dominfo = NULL;
- if (!get_dominfo(dom, &dominfo))
+ ret = get_dominfo(dom, &dominfo);
+ if (!ret)
goto out;
op = CMGetObjectPath(inst, NULL);
6:57 p.m.
KR> ret = _get_dominfo(xml, *dominfo);
KR> + if (!ret) {
KR> + free(*dominfo);
KR> + *dominfo = NULL;
KR> + return 0;
KR> + }
KR> +
KR> free(xml);
You've just introduced a memory leak here, since xml won't get
free()'d. Just remove the "return 0" and let it fall through and it
should be fine.
KR> diff -r 4ceb57b4430b -r f29660709900 src/Virt_VSSD.c
KR> --- a/src/Virt_VSSD.c Mon Nov 12 13:54:53 2007 -0800
KR> +++ b/src/Virt_VSSD.c Mon Nov 12 16:15:41 2007 -0800
KR> @@ -45,7 +45,8 @@ static int instance_from_dom(virDomainPt
KR> CMPIObjectPath *op;
KR> struct domain *dominfo = NULL;
KR> - if (!get_dominfo(dom, &dominfo))
KR> + ret = get_dominfo(dom, &dominfo);
KR> + if (!ret)
KR> goto out;
Maybe my eyes are tired at the end of the day, but does this fix
anything or is it just a style change?
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
7:18 p.m.
Dan Smith wrote:
KR> ret = _get_dominfo(xml, *dominfo);
KR> + if (!ret) {
KR> + free(*dominfo);
KR> + *dominfo = NULL;
KR> + return 0;
KR> + }
KR> +
KR> free(xml);
You've just introduced a memory leak here, since xml won't get
free()'d. Just remove the "return 0" and let it fall through and it
should be fine.
Yep, good call. Although, if we fall through here, we run into problems
with the (*dominfo)->dev_mem_ct = get_mem_devices(dom,
&(*dominfo)->dev_mem); related pieces since dominfo is NULL. Instead, I
think it should be:
ret = _get_dominfo(xml, *dominfo);
free(xml);
if (!ret) {
free(*dominfo);
*dominfo = NULL;
return 0;
}
KR> - if (!get_dominfo(dom, &dominfo))
KR> + ret = get_dominfo(dom, &dominfo);
KR> + if (!ret)
KR> goto out;
Maybe my eyes are tired at the end of the day, but does this fix
anything or is it just a style change?
We need to capture ret from get_domain(). ret is initialized to 1. If
we don't capture the ret from get_domain(), then we return 1 to
get_vssd_instance(). This causes the if statement to pass, and we
return an empty instance. If get_domain() fails, we need to return NULL.
--
Kaitlin Rupert
IBM Linux Technology Center
karupert(a)us.ibm.com
7:35 p.m.
KR> Yep, good call. Although, if we fall through here, we run into
KR> problems with the (*dominfo)->dev_mem_ct = get_mem_devices(dom,
KR> &(*dominfo)->dev_mem); related pieces since dominfo is NULL.
Sorry, I didn't have the full context.
I think it's bad form to cleanup in multiple exit paths. How about
moving the free(xml) to the end, and adding an "out" label right
before it. Then you can free(dominfo), set it to NULL, and goto out
to free(xml) and return ret, which will be zero, signalling error to
the caller. That would fit better with how we handle error paths
elsewhere in the code.
KR> We need to capture ret from get_domain().
Ah, right, okay, I see it now. Again, I wasn't looking at the context
of the rest of the file :)
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
6249
days inactive
6249
days old
3 comments
2 participants
participants (2)
-
Dan Smith -
Kaitlin Rupert