[Qemu-devel] Fwd: qemu code review

[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 <sgrubb@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.

Re: [Qemu-devel] Fwd: qemu code review

[malc]

On Wed, 18 Nov 2009, Kevin Wolf wrote:

> 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? ;-)

You mean translate-XXX? In a usual twisted way (in which nature
normally operates) i was trying, just for kicks, to build QEMU with
clang just yesterday, and it found one of the issues Steve review also
found (bt-sdp), but also something his review didn't bt-l2cap.c:1000

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Fwd: qemu code review

[Blue Swirl]

On Wed, Nov 18, 2009 at 1:39 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> 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? ;-)

> In target-sparc/helper.c at line 1277, "name" has not be checked for
> non-NULL
> value before use.

The check is done earlier in sun4m.c, sun4u.c or *-user/main.c.
There's always code like this:
    if (!cpu_model)
        cpu_model = hwdef->default_cpu_model;
or
    if (cpu_model == NULL) {
...
#elif defined(TARGET_SPARC)
#ifdef TARGET_SPARC64
        cpu_model = "TI UltraSparc II";
#else
        cpu_model = "Fujitsu MB86904";
#endif

before any call to cpu_init().

Re: [Qemu-devel] Fwd: qemu code review

[Stefan Weil]

Kevin Wolf schrieb:
> 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 <sgrubb@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 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.
>   

Obviously this was intentional. Would replacing
        memmove(tp->vlan, tp->data, 12);
by
        memmove(tp->data - 4, tp->data, 12);
be better and satisfy the analysis tool? Or even better
(hopefully the compiler will combine both statements)
        memmove(tp->vlan, tp->data, 4);
        memmove(tp->data, tp->data + 4, 8);

> 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.
>
> ...

Re: [Qemu-devel] Fwd: qemu code review

[Kevin Wolf]

Am 18.11.2009 20:06, schrieb Stefan Weil:
> Kevin Wolf schrieb:
>> 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 <sgrubb@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 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.
>>   
> 
> Obviously this was intentional. Would replacing
>         memmove(tp->vlan, tp->data, 12);
> by
>         memmove(tp->data - 4, tp->data, 12);
> be better and satisfy the analysis tool? Or even better
> (hopefully the compiler will combine both statements)
>         memmove(tp->vlan, tp->data, 4);
>         memmove(tp->data, tp->data + 4, 8);

I don't even know which analysis tool he used. Steve?

But I think splitting it into two memmoves is better anyway. There is no
warning in the declaration of the struct that these fields need to be
consecutive, so someone might have the idea of reordering the fields or
inserting a new one in between and things break...

Kevin

Re: [Qemu-devel] Fwd: qemu code review

[Steve Grubb]

On Thursday 19 November 2009 04:09:48 am Kevin Wolf wrote:
> >> ...
> >> 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.
> >
> > Obviously this was intentional. Would replacing
> >         memmove(tp->vlan, tp->data, 12);
> > by
> >         memmove(tp->data - 4, tp->data, 12);
> > be better and satisfy the analysis tool?

No. Its likely point out a negative index.

> > Or even better (hopefully the compiler will combine both statements)
> >         memmove(tp->vlan, tp->data, 4);
> >         memmove(tp->data, tp->data + 4, 8);

This would make it happier. But if a comment was made that its intentionally 
overrunning the vlan array, it would cause less concern.
 
> But I think splitting it into two memmoves is better anyway. There is no
> warning in the declaration of the struct that these fields need to be
> consecutive, so someone might have the idea of reordering the fields or
> inserting a new one in between and things break...

Right. Someone might use a cache analysis tool in the future and see that it 
runs faster with reordered fields...

-Steve

[Qemu-devel] [PATCH] e1000: Fix warning from code review

[Stefan Weil]

A code review run by Steve Grubb complained about code in e1000.c:

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.

This was fixed by splitting the memmove in two calls and
adding a comment to the declaration of vlan and data.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/e1000.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 00f6a57..6f9adb5 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -89,6 +89,7 @@ typedef struct E1000State_st {
     struct e1000_tx {
         unsigned char header[256];
         unsigned char vlan_header[4];
+        /* Fields vlan and data must not be reordered or separated. */
         unsigned char vlan[4];
         unsigned char data[0x10000];
         uint16_t size;
@@ -383,7 +384,8 @@ xmit_seg(E1000State *s)
     if (tp->sum_needed & E1000_TXD_POPTS_IXSM)
         putsum(tp->data, tp->size, tp->ipcso, tp->ipcss, tp->ipcse);
     if (tp->vlan_needed) {
-        memmove(tp->vlan, tp->data, 12);
+        memmove(tp->vlan, tp->data, 4);
+        memmove(tp->data, tp->data + 4, 8);
         memcpy(tp->data + 8, tp->vlan_header, 4);
         qemu_send_packet(s->vc, tp->vlan, tp->size + 4);
     } else
-- 
1.5.6.5

Re: [Qemu-devel] [PATCH] e1000: Fix warning from code review

[Ian Molton]

Stefan Weil wrote:
> A code review run by Steve Grubb complained about code in e1000.c:
> 
> 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.

> +        /* Fields vlan and data must not be reordered or separated. */
>          unsigned char vlan[4];
>          unsigned char data[0x10000];

Wouldnt it be better to stuff both into a struct or something? I guess
from the '12' that the data size can vary, but thats less important if
they are packed in a way that the compiler (and coders!) know not to
seperate them.

-Ian

Re: [Qemu-devel] Fwd: qemu code review

[Daniel P. Berrange]

On Thu, Nov 19, 2009 at 01:11:56PM -0500, Steve Grubb wrote:
> On Thursday 19 November 2009 04:09:48 am Kevin Wolf wrote:
> > >> ...
> > >> 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.
> > >
> > > Obviously this was intentional. Would replacing
> > >         memmove(tp->vlan, tp->data, 12);
> > > by
> > >         memmove(tp->data - 4, tp->data, 12);
> > > be better and satisfy the analysis tool?
> 
> No. Its likely point out a negative index.
> 
> > > Or even better (hopefully the compiler will combine both statements)
> > >         memmove(tp->vlan, tp->data, 4);
> > >         memmove(tp->data, tp->data + 4, 8);
> 
> This would make it happier. But if a comment was made that its intentionally 
> overrunning the vlan array, it would cause less concern.

Even if it was delibrate, it should be changed because when you build
with FORTIFY_SOURCE, glibc tries to add check for overruns in this type
of scenario and will abort the process. We're already being hit by aborts()
on these delibrate overruns in the vvfat block driver

  https://bugzilla.redhat.com/show_bug.cgi?id=538047


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|