[Qemu-devel] Fwd: qemu code review

[Kevin Wolf]

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 <address@hidden>

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.