| Message ID | 1467636059-12557-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 >> >> > > > . >
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
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>
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?
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
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.
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; }
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(-)