From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRMDT-00058Z-Ju for qemu-devel@nongnu.org; Thu, 04 Feb 2016 10:53:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRMDS-0006yr-OM for qemu-devel@nongnu.org; Thu, 04 Feb 2016 10:53:07 -0500 References: <1454542737-14742-1-git-send-email-jsnow@redhat.com> <20160204124333.GB2314@noname> From: John Snow Message-ID: <56B373DB.40209@redhat.com> Date: Thu, 4 Feb 2016 10:52:59 -0500 MIME-Version: 1.0 In-Reply-To: <20160204124333.GB2314@noname> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-img: initialize MapEntry object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: famz@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org 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 >> --- >> 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.