Message ID | 1454542737-14742-1-git-send-email-jsnow@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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>
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
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.
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;
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(-)