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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2014/10/30 15:10, Markus Armbruster wrote:
> <arei.gonglei@huawei.com> writes:
>
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> After commit 4c7e251a (), when dump memory completed,
>> the s->fd will be closed twice. We should return
>> directly when dump completed.
>>
>> Using do/while block, make the badly chosen return
>> values of get_next_block() more visible and fix
>> this issue.
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>
> 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?
>

Yep, this is really a stupid mistake i made when i do clean work for dump.c,
what lucky thing is there is no version release after the commit.
Thanks for your good catch.;)

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