[Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Paolo Bonzini]

Now that json-streamer tries not to leak tokens on incomplete parse,
the tokens can be freed twice if QEMU destroys the json-streamer
object during the parser->emit call.  To fix this, create the new
empty GQueue earlier, so that it is already in place when the old
one is passed to parser->emit.

Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qobject/json-streamer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index 7164390..c51c202 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -39,6 +39,7 @@ static void json_message_process_token(JSONLexer *lexer, GString *input,
 {
     JSONMessageParser *parser = container_of(lexer, JSONMessageParser, lexer);
     JSONToken *token;
+    GQueue *tokens;
 
     switch (type) {
     case JSON_LCURLY:
@@ -96,9 +97,12 @@ out_emit:
     /* send current list of tokens to parser and reset tokenizer */
     parser->brace_count = 0;
     parser->bracket_count = 0;
-    /* parser->emit takes ownership of parser->tokens.  */
-    parser->emit(parser, parser->tokens);
+    /* parser->emit takes ownership of parser->tokens.  Remove our own
+     * reference to parser->tokens before handing it out to parser->emit.
+     */
+    tokens = parser->tokens;
     parser->tokens = g_queue_new();
+    parser->emit(parser, tokens);
     parser->token_size = 0;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Fam Zheng]

On Mon, 07/04 14:40, Paolo Bonzini wrote:
> Now that json-streamer tries not to leak tokens on incomplete parse,
> the tokens can be freed twice if QEMU destroys the json-streamer
> object during the parser->emit call.  To fix this, create the new
> empty GQueue earlier, so that it is already in place when the old
> one is passed to parser->emit.
> 
> Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Two meta questions:

Is there a reproducer and/or test case coverage?

Does qemu-stable need this?

Fam

> ---
>  qobject/json-streamer.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
> index 7164390..c51c202 100644
> --- a/qobject/json-streamer.c
> +++ b/qobject/json-streamer.c
> @@ -39,6 +39,7 @@ static void json_message_process_token(JSONLexer *lexer, GString *input,
>  {
>      JSONMessageParser *parser = container_of(lexer, JSONMessageParser, lexer);
>      JSONToken *token;
> +    GQueue *tokens;
>  
>      switch (type) {
>      case JSON_LCURLY:
> @@ -96,9 +97,12 @@ out_emit:
>      /* send current list of tokens to parser and reset tokenizer */
>      parser->brace_count = 0;
>      parser->bracket_count = 0;
> -    /* parser->emit takes ownership of parser->tokens.  */
> -    parser->emit(parser, parser->tokens);
> +    /* parser->emit takes ownership of parser->tokens.  Remove our own
> +     * reference to parser->tokens before handing it out to parser->emit.
> +     */
> +    tokens = parser->tokens;
>      parser->tokens = g_queue_new();
> +    parser->emit(parser, tokens);
>      parser->token_size = 0;
>  }
>  
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Changlong Xie]

On 07/05/2016 02:56 PM, Fam Zheng wrote:
> On Mon, 07/04 14:40, Paolo Bonzini wrote:
>> Now that json-streamer tries not to leak tokens on incomplete parse,
>> the tokens can be freed twice if QEMU destroys the json-streamer
>> object during the parser->emit call.  To fix this, create the new
>> empty GQueue earlier, so that it is already in place when the old
>> one is passed to parser->emit.
>>
>> Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Two meta questions:
>
> Is there a reproducer and/or test case coverage?

tests/qemu-iotests/071

>
> Does qemu-stable need this?
>

http://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00465.html

Thanks
	-Xie

> Fam
>
>> ---
>>   qobject/json-streamer.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
>> index 7164390..c51c202 100644
>> --- a/qobject/json-streamer.c
>> +++ b/qobject/json-streamer.c
>> @@ -39,6 +39,7 @@ static void json_message_process_token(JSONLexer *lexer, GString *input,
>>   {
>>       JSONMessageParser *parser = container_of(lexer, JSONMessageParser, lexer);
>>       JSONToken *token;
>> +    GQueue *tokens;
>>
>>       switch (type) {
>>       case JSON_LCURLY:
>> @@ -96,9 +97,12 @@ out_emit:
>>       /* send current list of tokens to parser and reset tokenizer */
>>       parser->brace_count = 0;
>>       parser->bracket_count = 0;
>> -    /* parser->emit takes ownership of parser->tokens.  */
>> -    parser->emit(parser, parser->tokens);
>> +    /* parser->emit takes ownership of parser->tokens.  Remove our own
>> +     * reference to parser->tokens before handing it out to parser->emit.
>> +     */
>> +    tokens = parser->tokens;
>>       parser->tokens = g_queue_new();
>> +    parser->emit(parser, tokens);
>>       parser->token_size = 0;
>>   }
>>
>> --
>> 1.8.3.1
>>
>>
>
>
> .
>

Re: [Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Fam Zheng]

On Tue, 07/05 15:16, Changlong Xie wrote:
> On 07/05/2016 02:56 PM, Fam Zheng wrote:
> > On Mon, 07/04 14:40, Paolo Bonzini wrote:
> > > Now that json-streamer tries not to leak tokens on incomplete parse,
> > > the tokens can be freed twice if QEMU destroys the json-streamer
> > > object during the parser->emit call.  To fix this, create the new
> > > empty GQueue earlier, so that it is already in place when the old
> > > one is passed to parser->emit.
> > > 
> > > Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Two meta questions:
> > 
> > Is there a reproducer and/or test case coverage?
> 
> tests/qemu-iotests/071
> 
> > 
> > Does qemu-stable need this?
> > 
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00465.html

Get it! Thanks!

Fam

Re: [Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On 07/04/2016 06:40 AM, Paolo Bonzini wrote:
> Now that json-streamer tries not to leak tokens on incomplete parse,
> the tokens can be freed twice if QEMU destroys the json-streamer
> object during the parser->emit call.  To fix this, create the new
> empty GQueue earlier, so that it is already in place when the old
> one is passed to parser->emit.
> 
> Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qobject/json-streamer.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Now that json-streamer tries not to leak tokens on incomplete parse,
> the tokens can be freed twice if QEMU destroys the json-streamer
> object during the parser->emit call.  To fix this, create the new
> empty GQueue earlier, so that it is already in place when the old
> one is passed to parser->emit.
>
> Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Want me to do the pull request?

Re: [Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Paolo Bonzini]

On 06/07/2016 16:30, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Now that json-streamer tries not to leak tokens on incomplete parse,
>> the tokens can be freed twice if QEMU destroys the json-streamer
>> object during the parser->emit call.  To fix this, create the new
>> empty GQueue earlier, so that it is already in place when the old
>> one is passed to parser->emit.
>>
>> Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Want me to do the pull request?

I'm doing one tomorrow, so your choice.

Paolo

Re: [Qemu-devel] [PATCH] json-streamer: fix double-free on exiting during a parse

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/07/2016 16:30, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Now that json-streamer tries not to leak tokens on incomplete parse,
>>> the tokens can be freed twice if QEMU destroys the json-streamer
>>> object during the parser->emit call.  To fix this, create the new
>>> empty GQueue earlier, so that it is already in place when the old
>>> one is passed to parser->emit.
>>>
>>> Reported-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 
>> Want me to do the pull request?
>
> I'm doing one tomorrow, so your choice.

Please include it in your pull request then.