Re: [Qemu-trivial] [Qemu-devel] [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:
>> <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?
>
> Heh.  I was starring at all this last 20 minutes, re-reading the
> original v1 patch and your (Marcus) followup suggestion, trying to
> match the commit description with the actual happening and with the
> "no return" case which was before this patch.  Oh well.
>
> Yes, this looks correct indeed, we come to the same conclusion.
> But at this stage I really wonder if this is a -trivial material.

Distinguished old math professor does a proof on the blackboard.  At
some point he faces the audience and says "this is trivial".  Faces
blackboard, pauses.  "Is it trivial?"  Pauses again.  Storms out of the
classroom.  After ten minutes, he comes back and exclaims "it *is*
trivial!"

> (I can apply it to -trivial because no maintainer is listed for
> this file and because after some digging it becomes obvious).

Appreciated!