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

[Markus Armbruster]

<address@hidden> writes:

> From: Gonglei <address@hidden>
>
> 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 <address@hidden>

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.