Hi all, as Steve suggests, I'm forwarding the list of issues he found to the mailing list. I've already looked at a few points in the block code and sent patches. If everyone picks up one point, we should get through the list quickly. Who volunteers for the TCG ones? ;-)
Kevin -------- Original-Nachricht -------- Betreff: [virt-devel] qemu code review Datum: Tue, 17 Nov 2009 14:05:33 -0500 Von: Steve Grubb <sgr...@redhat.com> Hello, I took a few hours to run qemu through an analysis tool. Below are the results of checking everything. I don't interact with the qemu community and thought someone here might want to take these finding upstream. The review was against 0.11.0-11 in rawhide. Thanks, -Steve ----------------------------- In posix-aio-compat.c at line 229 is a test for nbytes < 0. This will never be true since nbytes is unsigned. In block/vvfat.c at line 267 is the definition of direntry_t which has a member called name which is 8 bytes in size. At line 445 there is an adjustment. So, "i" could be 21 yielding an offset of 25. At line 448, offset is used as an index into name (which is 8 bytes) and will be updating the wrong area in memory. At line 515, we have another error regarding the name array. The test for i <=8 should be < 8 since it is a zero based index. At line 615, we have a memset for an array sized 11 bytes, but name is only 8. The same problem shows up at 630 and 653. At line 1738 we have an array path2 sized to PATH_MAX. Suppose PATH_MAX is 4096 and the path being passed is 4095. The check at 1740 will pass since its legal. At line 1742, we add a '/' to the last location within path2. At line 1743, it adds a NUL to something outside of the path2 array. Path2 should be PATH_MAX+1 in size. In qemu-io.c at lines 122, 128, and 134 is a return without freeing sizes. In qemu-img.c at line 613 is a return without freeing bs. In console.c at line 462, font_data is set to 0xFFFF. At line 464, (font_data >> 4) is used to index into dmask16. However, at line 316, dmask16 is declared to be 16 in size. I suspect that font_data should be "anded" with 0x0F to make the index stay within its bounds. At line 477 is another occurance of a similar problem except dmask4 is an array of 4. In hw/bt-sdp.c at line 174 is an "if" statement with a ';' at the end. No code past 175 ever gets executed. In audio/audio_template.h at line 488 is a test for "sw" to be true. It is guaranteed to always be true and this test is not needed. In keymaps.c at line 108 is a test to see if rest is not NULL. It will never be NULL because its take from end_of_keysym + 1. So, if end_of_keysym was in fact NULL, rest would be a 1 and therefore not NULL. I think the test can safely be deleted. In vnc-auth-sasl.c at lines 447, 454, and 461 we return without freeing mechname. In slirp/ip_output.c at line 97 is an odd calculation for len. IF_MTU is 1500 and hlen comes from the sizeof struct ip. Suppose that it was 40. Then you would have 1460 &~ 0x07. The ~ makes the 0x07 into 0xFFFFFFF8. When this is anded with 1460, its guaranteed to be larger than 8. The calculation for len is completely wrong. In slirp/tcp_output.c at line 168 is an "if" statement with a 1 or'ed in. If this was intentional, there should be a comment or the "if" deleted. In vl.c at lines 2277, 2309, and 2317 we return without freeing devaddr. At line 5165 is a test for nb_numa_nodes >= MAX_NODES. However, its initialized to 0 at line 5009 and not changed. This test will always be false. At line 5935, 5940, and 5945 are tests for nb_drives_opt < MAX_DRIVES. nb_drives_opt is initialzed to 0 and nothing has changed it. In hw/rtl8139.c at line 1483 is an "if" statement with a 1 or'ed in. If this was intentional, there should be a comment. or the "if" stament removed. In hw/e1000.c at line 89, vlan is declared to be 4 bytes. At line 382 is an attempt to do a memmove over it with a size of 12. In hw/sb16.c at line 898 is an "if" statement with 0 and'ed. If this was intentional, there should be a comment or the code in the if statement deleted. In hw/cirrus_vga.c at line 2359 is an "if (0)" statement. The code should just be deleted if this was intentional. In target-i386/helper.c at line 491 is a test for model_id to not be NULL. It can never be NULL since model_id is an array within the x86_def_t structure. So, even if def is NULL, model_id will not be. Should this be testing for model_id[0] == 0? In the case of hw/realview_gic.c, it includes hw/arm_gic.c. At line 91, last_active is a two dimensional array. One bound is GIC_NIRQ which is defined as 96 in realview_gic. At line 212 is a test to see if irq i == 1023 - which means that it really could be. Suppose its value is 1022. Then at 227 it will be used as an index into the last_active array that is bound to 96 as the maximum value. Somewhere along the way, irq should be checked to see if its < GIC_NIRQ so that we don't index out of the array. In hw/armv7m_nvic.c, it includes hw/arm_gic.c. We have a similar problem to the one above at the same places. In hw/pl061.c at line 72 is a test for s->out being true. It will always be non-NULL because its an array inside pl061_state and not a pointer. In hw/omap2.c at line 2127 a 1 is being or'ed in an "if"statement. If this is intentional, there should be a comment or the "if" taken away so its just an else. At line 3002 an array of 2 elements is declared. At line 3010 is mode[2] which is out of bounds. In target-cris/translate.c at line 170 is a test if an index is > 15 or < 0. If so it prints an error message but goes ahead and indexes into the array anways. It probably should not do that. There are many cases of this in this source file. In target-m68k/helper.c at line 772 we find a ';' added to an "if" statement In hw/ppc_prep.c at line 618, there is a test for kernel_size < 0. kernel_size is an unsigned int and will never be < 0. At 627 is a similar problem regarding the initrd. In hw/ppc_newworld.c at lines 196, 199, 203, 212 we have the same issue as the one above. In target-sparc/helper.c at line 1277, "name" has not be checked for non-NULL value before use. In linux-user/syscall.c are a bunch of tests for addrlen < 0. Addrlen is socklen_t. Socklen_t is defined in /usr/include/bits/types.h as __U32_TYPE. None of the tests at 1501, 1520, 1606, 1634, 1662, 1703, and 1742 will work. At line 2499 host_mb is malloc'ed. If at line 2507 we goto end, host_mb is not freed. In linux-user/syscall.c at line 1286, we return withoout freeing "s". At 1289, 1293, and 1324, we return without freeing "syms" or "s". In linux-user/flatload.c at lines 495 and 550, result is checked for < 0. It is unsigned and will never be < 0. At line 544 its checked to be >= 0 and it will always be true.
