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

Max Reitz posted 1 patch 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170717151207.24919-1-mreitz@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
block/vvfat.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] block/vvfat: Fix compiler warning with gcc 7
Posted by Max Reitz 6 years, 8 months ago
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);
             memcpy(entry->name + MIN(j, 8 - len), tail, len);
         }
         for (entry1 = array_get(&(s->directory), directory_start);
-- 
2.9.4


Re: [Qemu-devel] [PATCH] block/vvfat: Fix compiler warning with gcc 7
Posted by Eric Blake 6 years, 8 months ago
On 07/17/2017 10:12 AM, Max Reitz wrote:
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] block/vvfat: Fix compiler warning with gcc 7
Posted by Kevin Wolf 6 years, 8 months ago
Am 17.07.2017 um 17:12 hat Max Reitz geschrieben:
> 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>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] block/vvfat: Fix compiler warning with gcc 7
Posted by Hervé Poussineau 6 years, 8 months ago
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.

>              memcpy(entry->name + MIN(j, 8 - len), tail, len);
>          }
>          for (entry1 = array_get(&(s->directory), directory_start);
>


Re: [Qemu-devel] [PATCH] block/vvfat: Fix compiler warning with gcc 7
Posted by Max Reitz 6 years, 8 months ago
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