Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] dump: fix use-after-free for s->fd

[Markus Armbruster]

Michael Tokarev <address@hidden> writes:

> 30.10.2014 10:10, Markus Armbruster wrote:
> []
>> I'm afraid the commit message is a bit misleading.  Let's examine what
>> exactly happens.
>> 
>> dump_iterate() dumps blocks in a loop.  Eventually, get_next_block()
>> returns "no more".  We then call dump_completed().  But we neglect to
>> break the loop!  Broken in commit 4c7e251a.
>> 
>> Because of that, we dump the last block again.  This attempts to write
>> to s->fd, which fails if we're lucky.  The error makes dump_iterate()
>> return unsuccessfully.  It's the only way it can ever return.
>> 
>> Theoretical: if we're not so lucky, something else has opened something
>> for writing and got the same fd.  dump_iterate() then keeps looping,
>> messing up the something else's output, until a write fails, or the
>> process mercifully terminates.
>> 
>> Is this correct?
>> 
>> If yes, let's use this commit message:
>> 
>>     dump: Fix dump-guest-memory termination and use-after-close
>> 
>>     dump_iterate() dumps blocks in a loop.  Eventually, get_next_block()
>>     returns "no more".  We then call dump_completed().  But we neglect to
>>     break the loop!  Broken in commit 4c7e251a.
>> 
>>     Because of that, we dump the last block again.  This attempts to write
>>     to s->fd, which fails if we're lucky.  The error makes dump_iterate()
>>     return failure.  It's the only way it can ever return.
>> 
>>     Theoretical: if we're not so lucky, something else has opened something
>>     for writing and got the same fd.  dump_iterate() then keeps looping,
>>     messing up the something else's output, until a write fails, or the
>>     process mercifully terminates.
>> 
>>     The obvious fix is to restore the return lost in commit 4c7e251a.  But
>>     the root cause of the bug is needlessly opaque loop control.  Replace it
>>     by a clean do ... while loop.
>> 
>>     This makes the badly chosen return values of get_next_block() more
>>     visible.  Cleaning that up is outside the scope of this bug fix.
>> 
>> You can then add my R-by.
>
> So I'm applying this -- which is your patch and your commit message, and
> I really wonder why this is Reviewed-by and not Signed-off-by, with your
> authorship?  It really should be...

You can add mine in addition to Gonglei's.

Signed-off-by: Markus Armbruster <address@hidden>