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