From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NXaI5-0000py-W0 for qemu-devel@nongnu.org; Wed, 20 Jan 2010 08:08:10 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NXaI0-0000o7-K6 for qemu-devel@nongnu.org; Wed, 20 Jan 2010 08:08:08 -0500 Received: from [199.232.76.173] (port=51908 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NXaI0-0000o1-CK for qemu-devel@nongnu.org; Wed, 20 Jan 2010 08:08:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35270) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NXaHz-0005ej-SR for qemu-devel@nongnu.org; Wed, 20 Jan 2010 08:08:04 -0500 Date: Wed, 20 Jan 2010 15:08:00 +0200 From: Gleb Natapov Subject: Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE Message-ID: <20100120130800.GH5238@redhat.com> References: <85e877202ec86dac15d392f5e88d5b5d76e3b02f.1263944807.git.quintela@redhat.com> <20100120103324.GA17856@redhat.com> <4B56ECBC.40804@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , "Kirill A. Shutemov" , qemu-devel@nongnu.org, Juan Quintela On Wed, Jan 20, 2010 at 02:03:04PM +0100, Markus Armbruster wrote: > "Kirill A. Shutemov" writes: >=20 > > On Wed, Jan 20, 2010 at 2:15 PM, Markus Armbruster = wrote: > >> Kevin Wolf writes: > >> > >>> Am 20.01.2010 12:09, schrieb Kirill A. Shutemov: > >>>> On Wed, Jan 20, 2010 at 12:33 PM, Daniel P. Berrange > >>>> wrote: > >>>>> On Wed, Jan 20, 2010 at 08:19:26AM +0200, Kirill A. Shutemov wrote: > >>>>>> On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela wrote: > >> [...] > >>>>>>> diff --git a/block/vvfat.c b/block/vvfat.c > >>>>>>> index 063f731..df957e5 100644 > >>>>>>> --- a/block/vvfat.c > >>>>>>> +++ b/block/vvfat.c > >>>>>>> @@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s, > >>>>>>> =9A =9A { > >>>>>>> =9A =9A =9A =9Adirentry_t* entry=3Darray_get_next(&(s->directory)= ); > >>>>>>> =9A =9A =9A =9Aentry->attributes=3D0x28; /* archive | volume labe= l */ > >>>>>>> - =9A =9A =9A snprintf((char*)entry->name,11,"QEMU VVFAT"); > >>>>>>> + =9A =9A =9A memcpy(entry->name,"QEMU VVF",8); > >>>>>>> + =9A =9A =9A memcpy(entry->extension,"AT ",3); > >>>>>>> =9A =9A } > >>>>>> > >>>>>> Better to use > >>>>>> > >>>>>> memcpy(entry->name, "QEMU VVFAT", 11); > >>>>>> > >>>>>> memcpy() doesn't check bounds. > >> > >> No, this is evil, and may well be flagged by static analysis tools. > > > > If so, the tool is stupid. > > > >>>>> It doesn't *currently* check bounds. > >>>> > >>>> No. memcpy() will never check bounds. It's totaly different from str= cpy, > >>>> http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html > >>> > >>> Regardless if deliberately overflowing the buffer works or doesn't > >>> making it explicit is better. Someone might reorder the struct or add > >>> new fields in between (okay, unlikely in this case, but still) and > >>> you'll overflow into fields you never wanted to touch. > >> > >> Moreover, compilers are free to put padding between members name and > >> extension. > > > > No, compiler can't add anything between. 'char' is always byte-aligned. >=20 > You got some reading to do then. >=20 To be fair this structure is packed, but this is not the reason to write stupid code of course. =20 -- Gleb.