Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

[Anthony Liguori]

On 01/20/2010 12:19 AM, Kirill A. Shutemov wrote:
> On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela<address@hidden>  wrote:
>    
> > From: Kirill A. Shutemov<address@hidden>
> >
> > CC    block/vvfat.o
> > cc1: warnings being treated as errors
> > block/vvfat.c: In function 'commit_one_file':
> > block/vvfat.c:2259: error: ignoring return value of 'ftruncate', declared with 
> > attribute warn_unused_result
> > make: *** [block/vvfat.o] Error 1
> >   CC    block/vvfat.o
> > In file included from /usr/include/stdio.h:912,
> >                  from ./qemu-common.h:19,
> >                  from block/vvfat.c:27:
> > In function 'snprintf',
> >     inlined from 'init_directories' at block/vvfat.c:871,
> >     inlined from 'vvfat_open' at block/vvfat.c:1068:
> > /usr/include/bits/stdio2.h:65: error: call to __builtin___snprintf_chk will 
> > always overflow destination buffer
> > make: *** [block/vvfat.o] Error 1
> >
> > Signed-off-by: Kirill A. Shutemov<address@hidden>
> > Signed-off-by: Juan Quintela<address@hidden>
> > ---
> >   block/vvfat.c |    9 +++++++--
> >   1 files changed, 7 insertions(+), 2 deletions(-)
> >
> > 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,
> >      {
> >         direntry_t* entry=array_get_next(&(s->directory));
> >         entry->attributes=0x28; /* archive | volume label */
> > -       snprintf((char*)entry->name,11,"QEMU VVFAT");
> > +       memcpy(entry->name,"QEMU VVF",8);
> > +       memcpy(entry->extension,"AT ",3);
> >      }
> >      
> Better to use
>
> memcpy(entry->name, "QEMU VVFAT", 11);
>
> memcpy() doesn't check bounds.
>    

Relying on such things is bad form because it isn't obvious to a casual 
reader what is happening.  You have to know that entry->name is 8 chars 
long and realize that it will overflow into extension.  Since that info 
is all the way in the structure definition, the result is difficult to 
read code.

Any other discussions about whether the standards allow such a thing or 
whether tools are "stupid" is irrelevant.  It's a neat trick but it 
results in more difficult to maintain code.

Regards,

Anthony Liguori