On Thu, Nov 05, 2009 at 01:12:41PM -0500, Steve Grubb wrote:
Hello,
The following is against 0.7.2-3 from F-13 cvs. I mention each source file at the beginning of a block and have a blank line indicating that we are moving along to the next file. Hope you find this helpful.
-Steve
Okay I fixed the issues on a 0.7.2 branch and then backported the patch back onto git head, except for a few exceptions, everything was still applying !
In src/util/storage_file.c at line 192, there is a test that I presume is for -1. The problem is that its a variable that is an unsigned long is 64 bits on
unsigned long size; size = ((buf[4+4+8] << 24) | (buf[4+4+8+1] << 16) | (buf[4+4+8+2] << 8) | buf[4+4+8+3]); /* QCowHeader.backing_file_size */ [...] if (size + 1 == 0) return BACKING_STORE_INVALID; if (VIR_ALLOC_N(*res, size + 1) < 0) { I think it's just to make 100% sure the size + 1 computation on the following line won't overflow. So current code looks fine to me
some arches. It would probably be better to use a unit32_t for size or change the test. The bottom line is the test will evaluate false on some arches.
unit32_t is nowhere used in libvirt code I would rather not use it, we could use size_t there since the goal is to allocate memory but I'm not 100% sure it's right, basically the test make sure we won't try to allocate more than 4G of memory in one chunk.
In src/datatypes.c at lines 335, 476, and 790, there is a test to see if uuid is NULL. There was a test in the beginning of the function and it returned if this was true, so this "if" statement can be deleted in all 3 cases.
Hum, this is the virGetxxx() functions, they lookup objects given the name or uuid, and hold big TODOs comments at the start: /* TODO search by UUID first as they are better differenciators */ /* TODO check the UUID */ And somehow I think the intend was to allow lookup only by name, with the uuid being unknown, I would think the proper fix is actually to drop the || (uuid == NULL) at the start of the function.
In src/libvirt.c at line 1122, there is an "if" statement that "ands" 0 with the return from STREQ. Should that have been a == ? The test evaluates false as is.
Comparing with others similar message being emitted in other places, I think '0 && ' is just a bogus remain, I'm removing it.
At line 3214, there is a test if uri == NULL. Then within that "if" statement at line 3216 is a test for if uri is still NULL. Should that have been a test for dstURI being NULL?
yup ! That had been fixed in head since though.
In src/remote/remote_driver.c at line 939 is a do loop. By design, it seems the intention was to reloop if waitpid returns -1 and EINTR is set. The implementation doesn't match it. If -1 is returned, it will do the continue, which is to jump to the end of the do loop and it evaluates reap which would be a -1 and it will exit. I suspect instead of continue, it should "goto again" and again would be placed before the call to waitpid. This problem is found again at line 1406
okay, nasty, somehow it's easy to think continue will retry from top of the loop not including the test
In src/xen/sexpr.c at line 443 is a test for token == NULL. Token is guaranteed to not be NULL because of the loop entry test at line 440.
okay
In src/xen/xend_internal.c at line 1367 is a test for connectPort being true. Prior to this, connectPort is guranteed to be valid or it would have jumped to no_memory.
okay as the affectation is done in both if and else parts of the preceeding test.
In src/xen/xm_internal.c at line 579 is a test for dh being valid. It is guaranteed to be valid since a -1 was returned from the function on a failed call to opendir.
okay
At line 2219 is an unusual if statement. Normally you do not see something constructed as (!cpu)<0). That would seem to have meant cpu>=0 which is more straightforward.
if (def->cpumask && !(cpus = virDomainCpuSetFormat(conn, def->cpumask, def->cpumasklen)) < 0) goto cleanup; But the function returns a string or NULL in case of error, I try to fight again not fully parenthetized test expressions but I'm loosing that fight unfortunately :-( in any case the < 0 test is bogus since it's a string if ((def->cpumask) && ((cpus = virDomainCpuSetFormat(conn, def->cpumask, def->cpumasklen)) == NULL)) goto cleanup seems the right construct to me, as it tests if the call failed.
In src/phyp/phyp_driver.c at line 1008, there is a strdup inside an if statement with nothing catching its returned memory. Two lines later it does the same thing again but puts it in an array. Not sure what the if statement should have been checking, but its definitely a memory leak as is. At line 1040 is an allocation. If that fails, it passes dom->comm to an error reporting function. However, at line 1034, dom was set to NULL and is not changed. The same problem appears again at line 1085. At line 1105 is a test for exit_status < 0. Nothing has changed it since it was initialized.
That code was completely revamped since, none of this applies now, it was pretty bad ! C.f. https://bugzilla.redhat.com/show_bug.cgi?id=529977
In src/openvz/openvz_conf at line 336 is a test for from being NULL. However, it was previously used at line 333 before checking if it was NULL.
yeah, there was another passed pointer which should have got the same check too, cleaning this up.
In src/qemu/qemu_monitor_text.c at line 178, a variable, control, is assigned to a member of the msg structure. However, control goes out of scope before its used. It should be in the function level declarations.
Ah right, nasty !
In src/qemu/qemu_driver.c at line 894 is a test for retries being 0. This will always be true since the loop has no break statements. At line 4484, we leave the function without freeing devname.
okay,
In src/lxc/lxc_conf.c at line 111, 114, and 125, we leave the function without freeing filename.
okay
In src/lxc/lxc_container.c at line 743, we leave the function without freeing ttyPath.
okay
In src/storage/storage_driver.c at lines 83 and 93 we call storageLog/fprintf with the second parameter to %s being NULL. Wasn't an empty string intended?
I used "no error message found" as we failed to gather an error message
In src/storage/storage_backend_disk.c at line 88, we leave the function without freeing devpath.
okay
At line 649 is a test for part_num being NULL. Can it ever be NULL?
No, but we should check for an empty string, done
In src/node_device/node_device_hal.c at line 405 is a test for hal_cap_names being true, its conceivable to get there without it being initialized. For example, line 373 could jump to it. It should probably be initialized to 0.
okay
At line 781 is a test for udi being true. It will alwys be false since once its defined it will never jump to failure. At line 782 is a use of num_devs without it being initialized. If the problem mentioned for udi is fixed by deleting all this code, then the problem goes away.
okay removing that block, I just hope we won't add a failure jump from within the loop
In src/lxc/lxc_controller.c at line 633 is a test for containerPty not being -1. Its conceivable to get here with it notr being initialized perhaps at line 512 for example. It should probably be initialized to a -1.
yup, okay
At line 735 is an if statement "anding" 0 with getuid(). this should probably be a != rather than a &&.
right since the error message clearly indicate we expect this to run as root. Thanks a lot for the review ! Patch for git head attached 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/