[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: |
Gonglei |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] dump: fix use-after-free for s->fd |
Date: |
Thu, 30 Oct 2014 18:50:18 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
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