On 6/22/11 5:08 AM, Jonas Gorski wrote:
> On 22 June 2011 12:56, Alexey I. Froloff <[email protected]
> <mailto:[email protected]>> wrote:
>> When host gcc uses _FORTIFY_SOURCE=2 by default, tools/firmware-utils
>> fails to compile:
>>
>> In file included from /usr/include/string.h:658:0,
>> from src/imagetag.c:12:
>> In function 'strncpy',
>> inlined from 'tagfile' at src/imagetag.c:369:11:
>> /usr/include/bits/string3.h:123:3: error: call to __builtin___strncpy_chk
>> will always overflow destination buffer
>>
>> Signed-off-by: Alexey I. Froloff <[email protected]
>> <mailto:[email protected]>>
>> ---
>> tools/firmware-utils/src/imagetag.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/firmware-utils/src/imagetag.c
>> b/tools/firmware-utils/src/imagetag.c
>> index bebaba2..cfe7cf4 100644
>> --- a/tools/firmware-utils/src/imagetag.c
>> +++ b/tools/firmware-utils/src/imagetag.c
>> @@ -362,11 +362,11 @@ int tagfile(const char *kernel, const char *rootfs,
>> const char *bin, \
>> }
>>
>> if (args->reserved2_given) {
>> - strncpy(tag.reserved2, args->reserved2_arg, 16);
>> + memcpy(tag.reserved2, args->reserved2_arg, 16);
>
> NACK. First of all, this didn't generate a warning at all (but that doesn't
> mean there can't a problem ;-). Secondly, you discard the null terminator if
> args->reserved2_arg is longer than 15 chars, creating potential problems
> later on. If it is less than 15, you'll read from the memory that comes after
> arg->reserved2_arg.
Actually, you discard the NUL termination either way. From strncpy(3c):
The strncpy() function is similar, except that at most n bytes of src
are copied. Warning: If there is no null byte among the first n bytes
of src, the string placed in dest will not be null-terminated.
>
>> }
>>
>> if (args->altinfo_given) {
>> - strncpy(&tag.information1[0], args->altinfo_arg, ALTTAGINFO_LEN);
>> + memcpy(&tag.information1[0], args->altinfo_arg, ALTTAGINFO_LEN);
>
> NACK for the same reasons above.
>
> Also the warning is mostly harmless; removing it isn't quite as easy. A
> potential solution would be to define a union struct with either
> information1, flashlayourver fsKernelCRC and information2 or a
> ALTTAGINFO_LEN sized altinfo, then using them as needed in struct bcm_tag, or
> make the bcm_tag itself a union of the different layouts.
>
> An actual bug is the length argument, it should be actually one less than
> _LEN (to leave space for the null terminator when the string is too long).
> This is harmless though, this can never happen since ALTTAGINFO_LEN long
> altinfos already get rejected earlier.
>
>
> Jonas
So use ALTTAGINFO_LEN - 1 sounds like the fix? But even then, you'd still need
to set the last character to NUL as above.
So just use strlcpy() it sounds like.
-Philip
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
