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

[Gonglei]

On 2014/10/30 17:23, Markus Armbruster wrote:

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

Do I need to send v3 for changing commit message, or
/mjt do it directly? Thanks

Best regards,
-Gonglei