[PATCH] This fixes a potential crash if _get_domain() is unable to properly parse the dom XML
# HG changeset patch # User Kaitlin Rupert <karupert@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@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);
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@us.ibm.com
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@us.ibm.com
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@us.ibm.com
participants (2)
-
Dan Smith -
Kaitlin Rupert