Re: [Qemu-devel] [PATCH v4 7/7] block/nbd: NBDReply is used being uninitialized

From: Eric Blake <eblake@redhat.com>
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: kwolf@redhat.com, vsementsov@virtuozzo.com, mreitz@redhat.com,
	den@openvz.org, "Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v4 7/7] block/nbd: NBDReply is used being uninitialized
Date: Fri, 19 Jul 2019 09:44:36 -0500	[thread overview]
Message-ID: <e12c0aa2-6757-856a-bbe0-717895453986@redhat.com> (raw)
In-Reply-To: <a4214009-e9ba-2f67-138b-30ce31f7905f@redhat.com>


[-- Attachment #1.1: Type: text/plain, Size: 1660 bytes --]

On 7/19/19 9:34 AM, Eric Blake wrote:
> On 7/19/19 4:40 AM, Andrey Shinkevich wrote:
>> In case nbd_co_receive_one_chunk() fails in
>> nbd_reply_chunk_iter_receive(), 'NBDReply reply' parameter is used in
>> the check nbd_reply_is_simple() without being initialized. The iotest
>> 083 does not pass under the Valgrind: $./check -nbd -valgrind 083.
>> The alternative solution is to swap the operands in the condition:
>> 'if (s->quit || nbd_reply_is_simple(reply))'
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>  block/nbd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Huh. Very similar to
> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html, but
> affects a different function. I can queue this one through my NBD tree
> to get both in my rc2 pull request.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Actually, since this is the second patch on the same topic, I'm
wondering if it's better to use the following one-liner to fix BOTH
issues and without relying on a gcc extension:

diff --git i/block/nbd.c w/block/nbd.c
index 8d565cc624ec..f751a8e633e5 100644
--- i/block/nbd.c
+++ w/block/nbd.c
@@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk(
                                           request_ret, qiov, payload,
errp);

     if (ret < 0) {
+        memset(reply, 0, sizeof *reply);
         s->quit = true;
     } else {
         /* For assert at loop start in nbd_connection_entry */

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


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