Re: [Qemu-devel] [PATCH] block/vvfat: Fix compiler warning with gcc 7

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

On 2017-07-17 20:40, Hervé Poussineau wrote:
> Le 17/07/2017 à 17:12, Max Reitz a écrit :
>> gcc 7 complains that the sprintf() might write a null byte beyond the
>> end of the tail buffer.  That is wrong, but we can silence it by making
>> i unsigned (it can never be negative anyway, see the if condition right
>> before).  For some reason, this allows gcc to suddenly accurately
>> calculate the range of i so we can give the tail[] array the exact size
>> it needs to have (which is 8 bytes) without gcc complaining.
>>
>> In addition, let us convert the sprintf() to snprintf(), because that is
>> always nicer, and add an assertion about the range of the return value
>> afterwards so we can see that "8 - len" will never be negative and thus
>> "entry->name + MIN(j, 8 - len)" will never be out of bounds.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/vvfat.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 6b11596..a9e207f 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -549,7 +549,7 @@ static direntry_t
>> *create_short_filename(BDRVVVFATState *s,
>>      const gchar *p, *last_dot = NULL;
>>      gunichar c;
>>      bool lossy_conversion = false;
>> -    char tail[11];
>> +    char tail[8];
>>
>>      if (!entry) {
>>          return NULL;
>> @@ -614,7 +614,8 @@ static direntry_t
>> *create_short_filename(BDRVVVFATState *s,
>>      for (i = lossy_conversion ? 1 : 0; i < 999999; i++) {
>>          direntry_t *entry1;
>>          if (i > 0) {
>> -            int len = sprintf(tail, "~%d", i);
>> +            int len = snprintf(tail, sizeof(tail), "~%u", (unsigned)i);
>> +            assert(len <= 7);
> 
> As i is on minimum between 0 or 1 and on maximum equal at 999999, does
> it work if you change the type of i from int to unsigned int?
> That way, you probably won't need the cast to unsigned in the s(n)printf.

Hm... It works in a way, but then gcc likes to think tail[] needs to be
9 bytes long (for whatever reason). So... It works in a sense, but not
quite as well.

So I'm not quite sure which way is better. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]