[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] dump: fix use-after-free for
From: |
Michael Tokarev |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] dump: fix use-after-free for s->fd |
Date: |
Thu, 30 Oct 2014 16:54:44 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.8.1 |
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