Patchwork [07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

login
register
mail settings
Submitter Juan Quintela
Date Jan. 19, 2010, 11:56 p.m.
Message ID <85e877202ec86dac15d392f5e88d5b5d76e3b02f.1263944807.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/43244/
State New
Headers show

Comments

Juan Quintela - Jan. 19, 2010, 11:56 p.m.
From: Kirill A. Shutemov <kirill@shutemov.name>

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 <kirill@shutemov.name>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/vvfat.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Kirill A. Shutemov - Jan. 20, 2010, 6:19 a.m.
On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela <quintela@redhat.com> wrote:
> From: Kirill A. Shutemov <kirill@shutemov.name>
>
> 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 <kirill@shutemov.name>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  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.

>     /* Now build FAT, and write back information into directory */
> @@ -2256,7 +2257,11 @@ static int commit_one_file(BDRVVVFATState* s,
>        c = c1;
>     }
>
> -    ftruncate(fd, size);
> +    if (ftruncate(fd, size)) {
> +        perror("ftruncate()");
> +        close(fd);
> +        return -4;
> +    }
>     close(fd);
>
>     return commit_mappings(s, first_cluster, dir_index);
> --
> 1.6.5.2
>
>
Daniel P. Berrange - Jan. 20, 2010, 10:33 a.m.
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 <quintela@redhat.com> wrote:
> > From: Kirill A. Shutemov <kirill@shutemov.name>
> >
> > 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 <kirill@shutemov.name>
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  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.

It doesn't *currently* check bounds. If we want to explicitly
fill 2 fields at once, then we should redeclare this to have a 
union with one part comprising the entire buffer, thus avoiding
the need for delibrate buffer overruns.

Regards,
Daniel
Kirill A. Shutemov - Jan. 20, 2010, 11:09 a.m.
On Wed, Jan 20, 2010 at 12:33 PM, Daniel P. Berrange
<berrange@redhat.com> 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 <quintela@redhat.com> wrote:
>> > From: Kirill A. Shutemov <kirill@shutemov.name>
>> >
>> > 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 <kirill@shutemov.name>
>> > Signed-off-by: Juan Quintela <quintela@redhat.com>
>> > ---
>> >  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.
>
> It doesn't *currently* check bounds.

No. memcpy() will never check bounds. It's totaly different from strcpy,
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html

> If we want to explicitly
> fill 2 fields at once, then we should redeclare this to have a
> union with one part comprising the entire buffer, thus avoiding
> the need for delibrate buffer overruns.
>
> Regards,
> 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 :|
>
Kevin Wolf - Jan. 20, 2010, 11:45 a.m.
Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
> On Wed, Jan 20, 2010 at 12:33 PM, Daniel P. Berrange
> <berrange@redhat.com> 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 <quintela@redhat.com> wrote:
>>>> From: Kirill A. Shutemov <kirill@shutemov.name>
>>>>
>>>> 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 <kirill@shutemov.name>
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>>  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.
>>
>> It doesn't *currently* check bounds.
> 
> No. memcpy() will never check bounds. It's totaly different from strcpy,
> 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.

Kevin
Markus Armbruster - Jan. 20, 2010, 12:15 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
>> On Wed, Jan 20, 2010 at 12:33 PM, Daniel P. Berrange
>> <berrange@redhat.com> 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 <quintela@redhat.com> 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,
>>>>>     {
>>>>>        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.

No, this is evil, and may well be flagged by static analysis tools.

>>> It doesn't *currently* check bounds.
>> 
>> No. memcpy() will never check bounds. It's totaly different from strcpy,
>> 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.
Kirill A. Shutemov - Jan. 20, 2010, 12:36 p.m.
On Wed, Jan 20, 2010 at 2:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
>>> On Wed, Jan 20, 2010 at 12:33 PM, Daniel P. Berrange
>>> <berrange@redhat.com> 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 <quintela@redhat.com> 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,
>>>>>>     {
>>>>>>        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.
>
> 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 strcpy,
>>> 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.
Markus Armbruster - Jan. 20, 2010, 1:03 p.m.
"Kirill A. Shutemov" <kirill@shutemov.name> writes:

> On Wed, Jan 20, 2010 at 2:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
>>>> On Wed, Jan 20, 2010 at 12:33 PM, Daniel P. Berrange
>>>> <berrange@redhat.com> 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 <quintela@redhat.com> 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,
>>>>>>>     {
>>>>>>>        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.
>>
>> 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 strcpy,
>>>> 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.

You got some reading to do then.
Gleb Natapov - Jan. 20, 2010, 1:08 p.m.
On Wed, Jan 20, 2010 at 02:03:04PM +0100, Markus Armbruster wrote:
> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
> 
> > On Wed, Jan 20, 2010 at 2:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >>> Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
> >>>> On Wed, Jan 20, 2010 at 12:33 PM, Daniel P. Berrange
> >>>> <berrange@redhat.com> 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 <quintela@redhat.com> 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,
> >>>>>>>     {
> >>>>>>>        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.
> >>
> >> 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 strcpy,
> >>>> 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.
> 
> You got some reading to do then.
> 
To be fair this structure is packed, but this is not the reason to write
stupid code of course.
 
--
			Gleb.
Kirill A. Shutemov - Jan. 20, 2010, 1:51 p.m.
On Wed, Jan 20, 2010 at 3:03 PM, Markus Armbruster <armbru@redhat.com> wrote:
> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
>> No, compiler can't add anything between. 'char' is always byte-aligned.
>
> You got some reading to do then.

Do you want to say that it's not true? Could you provide an example
when 'char' isn't byte-aligned?
Kirill A. Shutemov - Jan. 20, 2010, 2:02 p.m.
2010/1/20 Gleb Natapov <gleb@redhat.com>:
> On Wed, Jan 20, 2010 at 02:03:04PM +0100, Markus Armbruster wrote:
>> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
>> > No, compiler can't add anything between. 'char' is always byte-aligned.
>>
>> You got some reading to do then.
>>
> To be fair this structure is packed, but this is not the reason to write
> stupid code of course.

In what way does it stupid? Compiler can't insert pads between two
arrays of 'char'.
Gleb Natapov - Jan. 20, 2010, 2:11 p.m.
On Wed, Jan 20, 2010 at 04:02:19PM +0200, Kirill A. Shutemov wrote:
> 2010/1/20 Gleb Natapov <gleb@redhat.com>:
> > On Wed, Jan 20, 2010 at 02:03:04PM +0100, Markus Armbruster wrote:
> >> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
> >> > No, compiler can't add anything between. 'char' is always byte-aligned.
> >>
> >> You got some reading to do then.
> >>
> > To be fair this structure is packed, but this is not the reason to write
> > stupid code of course.
> 
> In what way does it stupid? Compiler can't insert pads between two
> arrays of 'char'.
Because this is not wise write tricky code especially if it buys you
nothing. This is not C obfuscation contest and it will bite you later.
What to search where extension[] is initialized? "grep extension" will
not find it! Need to change structure layout? Have to change unrelated
code too, but, of course, can't even know that. So can you please tell
us why that code should stay like it is now?

--
			Gleb.
Markus Armbruster - Jan. 20, 2010, 2:42 p.m.
[Some quoted material restored]

"Kirill A. Shutemov" <kirill@shutemov.name> writes:

> On Wed, Jan 20, 2010 at 3:03 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
>>> On Wed, Jan 20, 2010 at 2:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>>> 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.
>>
>> You got some reading to do then.
>
> Do you want to say that it's not true? Could you provide an example
> when 'char' isn't byte-aligned?

I was wrong, because I overlooked __attribute__((packed)).

ISO/IEC 9899:1999 6.7.2.1 guarantees a number of things for structs,
chiefly members stay in order, no padding before the first member.  But
it does not restrict padding between members or at the end in any way,
so compilers may pad there how they see fit.  In particular, there is no
rule that a compiler may only add padding to satisfy the next member's
alignment requirement.

__attribute__((packed)) tightens the spec to eliminate padding.

Regardless, the deliberate buffer overflow to hopefully save a few bytes
and cycles is a dirty, brittle trick for no good reason.  If this really
must be hand-optimized (numbers, please), do it cleanly, the way Dan
suggested.
malc - Jan. 20, 2010, 3 p.m.
On Wed, 20 Jan 2010, Kirill A. Shutemov wrote:

> 2010/1/20 Gleb Natapov <gleb@redhat.com>:
> > On Wed, Jan 20, 2010 at 02:03:04PM +0100, Markus Armbruster wrote:
> >> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
> >> > No, compiler can't add anything between. 'char' is always byte-aligned.
> >>
> >> You got some reading to do then.
> >>
> > To be fair this structure is packed, but this is not the reason to write
> > stupid code of course.
> 
> In what way does it stupid? Compiler can't insert pads between two
> arrays of 'char'.

Can you cite chapter and verse of the standard where it's spelled out?
Anthony Liguori - Jan. 20, 2010, 3:59 p.m.
On 01/20/2010 12:19 AM, Kirill A. Shutemov wrote:
> On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela<quintela@redhat.com>  wrote:
>    
>> From: Kirill A. Shutemov<kirill@shutemov.name>
>>
>> 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<kirill@shutemov.name>
>> Signed-off-by: Juan Quintela<quintela@redhat.com>
>> ---
>>   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
Jamie Lokier - Jan. 20, 2010, 4:16 p.m.
Kirill A. Shutemov wrote:
> On Wed, Jan 20, 2010 at 3:03 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > "Kirill A. Shutemov" <kirill@shutemov.name> writes:
> >> No, compiler can't add anything between. 'char' is always byte-aligned.
> >
> > You got some reading to do then.
> 
> Do you want to say that it's not true? Could you provide an example
> when 'char' isn't byte-aligned?

I have no example, but that doesn't mean there is no architecture, ABI
or host OS where it happens.  Architectures can be surprising.

It's very easy to make assumptions which break on some architecture
you've never tested on.

Take this code:

struct part1 {
	char c[5];
};

struct part2 {
	char c[3];
};

struct all {
	struct part1 p1;
	struct part2 p2;
};

What do you think the size is on ARM?  Hint: not 8 when using GCC < 4,
but it is 8 when using current GCC and the later ARM ABI.

This broke some data parsing application when it was ported from x86
to ARM, because of a wrong assumption about structure layout that
works on nearly all architectures and passed all testing prior to the port.

-- Jamie

Patch

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

     /* Now build FAT, and write back information into directory */
@@ -2256,7 +2257,11 @@  static int commit_one_file(BDRVVVFATState* s,
 	c = c1;
     }

-    ftruncate(fd, size);
+    if (ftruncate(fd, size)) {
+        perror("ftruncate()");
+        close(fd);
+        return -4;
+    }
     close(fd);

     return commit_mappings(s, first_cluster, dir_index);