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

[Michael Tokarev <mjt@tls.msk.ru>]

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...

Thanks,

/mjt