[Qemu-devel] [PATCH] qemu-img: initialize MapEntry object

[Qemu-devel] [PATCH] qemu-img: initialize MapEntry object

[John Snow]

Commit 16b0d555 introduced an issue where we are not initializing
has_filename for the 'next' MapEntry object, which leads to interesting
errors in Valgrind and Clang -fsanitize=undefined both.

Zero the stack object at allocation AND make sure the utility to
populate the fields properly marks has_filename as false if applicable.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-img.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index f121980..5a85178 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2231,6 +2231,9 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
     if (file && e->has_offset) {
         e->has_filename = true;
         e->filename = file->filename;
+    } else {
+        e->has_filename = false;
+        e->filename = NULL;
     }
     return 0;
 }
@@ -2264,7 +2267,7 @@ static int img_map(int argc, char **argv)
     BlockDriverState *bs;
     const char *filename, *fmt, *output;
     int64_t length;
-    MapEntry curr = { .length = 0 }, next;
+    MapEntry curr = { .length = 0 }, next = { .length = 0 };
     int ret = 0;
 
     fmt = NULL;
-- 
2.4.3

Re: [Qemu-devel] [PATCH] qemu-img: initialize MapEntry object

[Fam Zheng]

On Wed, 02/03 18:38, John Snow wrote:
> Commit 16b0d555 introduced an issue where we are not initializing
> has_filename for the 'next' MapEntry object, which leads to interesting
> errors in Valgrind and Clang -fsanitize=undefined both.
> 
> Zero the stack object at allocation AND make sure the utility to
> populate the fields properly marks has_filename as false if applicable.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qemu-img.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index f121980..5a85178 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2231,6 +2231,9 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
>      if (file && e->has_offset) {
>          e->has_filename = true;
>          e->filename = file->filename;
> +    } else {
> +        e->has_filename = false;
> +        e->filename = NULL;
>      }
>      return 0;
>  }
> @@ -2264,7 +2267,7 @@ static int img_map(int argc, char **argv)
>      BlockDriverState *bs;
>      const char *filename, *fmt, *output;
>      int64_t length;
> -    MapEntry curr = { .length = 0 }, next;
> +    MapEntry curr = { .length = 0 }, next = { .length = 0 };
>      int ret = 0;
>  
>      fmt = NULL;
> -- 
> 2.4.3
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH] qemu-img: initialize MapEntry object

[Kevin Wolf]

Am 04.02.2016 um 00:38 hat John Snow geschrieben:
> Commit 16b0d555 introduced an issue where we are not initializing
> has_filename for the 'next' MapEntry object, which leads to interesting
> errors in Valgrind and Clang -fsanitize=undefined both.
> 
> Zero the stack object at allocation AND make sure the utility to
> populate the fields properly marks has_filename as false if applicable.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qemu-img.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index f121980..5a85178 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2231,6 +2231,9 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
>      if (file && e->has_offset) {
>          e->has_filename = true;
>          e->filename = file->filename;
> +    } else {
> +        e->has_filename = false;
> +        e->filename = NULL;
>      }
>      return 0;
>  }

I guess this fixes the bug, but wouldn't it actually be nicer to just
reinitialise the whole object? As everyone knows, I love compound
literals, so I'd make it one big assignment that zeroes everything that
isn't specified:

*e = (MapEntry) {
    ...
};

> @@ -2264,7 +2267,7 @@ static int img_map(int argc, char **argv)
>      BlockDriverState *bs;
>      const char *filename, *fmt, *output;
>      int64_t length;
> -    MapEntry curr = { .length = 0 }, next;
> +    MapEntry curr = { .length = 0 }, next = { .length = 0 };
>      int ret = 0;

At first I didn't quite understand what this was for, but I think you
tried to cover newly added fields. If you overwrite the whole struct
above, you wouldn't need to initialise it here any more.

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: initialize MapEntry object

[John Snow]

On 02/04/2016 07:43 AM, Kevin Wolf wrote:
> Am 04.02.2016 um 00:38 hat John Snow geschrieben:
>> Commit 16b0d555 introduced an issue where we are not initializing
>> has_filename for the 'next' MapEntry object, which leads to interesting
>> errors in Valgrind and Clang -fsanitize=undefined both.
>>
>> Zero the stack object at allocation AND make sure the utility to
>> populate the fields properly marks has_filename as false if applicable.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qemu-img.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index f121980..5a85178 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2231,6 +2231,9 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
>>      if (file && e->has_offset) {
>>          e->has_filename = true;
>>          e->filename = file->filename;
>> +    } else {
>> +        e->has_filename = false;
>> +        e->filename = NULL;
>>      }
>>      return 0;
>>  }
> 
> I guess this fixes the bug, but wouldn't it actually be nicer to just
> reinitialise the whole object? As everyone knows, I love compound
> literals, so I'd make it one big assignment that zeroes everything that
> isn't specified:
> 
> *e = (MapEntry) {
>     ...
> };
> 

Yeah, that's more future proof isn't it.

>> @@ -2264,7 +2267,7 @@ static int img_map(int argc, char **argv)
>>      BlockDriverState *bs;
>>      const char *filename, *fmt, *output;
>>      int64_t length;
>> -    MapEntry curr = { .length = 0 }, next;
>> +    MapEntry curr = { .length = 0 }, next = { .length = 0 };
>>      int ret = 0;
> 
> At first I didn't quite understand what this was for, but I think you
> tried to cover newly added fields. If you overwrite the whole struct
> above, you wouldn't need to initialise it here any more.
> 
> Kevin
> 

Yeah, the intent was:

(1) Always make sure it starts out zeroed.
(2) In case someone tries to call get_block_status with an object that
isn't zeroed in the future, make sure has_filename is toggled back to false.



I'll send you a new one.