Re: [PATCH v2] json: Fix a memleak in parse_pair()

[Markus Armbruster]

"Chenqun (kuhn)" <kuhn.chenqun@huawei.com> writes:

>> -----Original Message-----
>> From: Chenzhendong (alex)
>> Sent: Friday, November 13, 2020 10:55 PM
>> To: armbru@redhat.com
>> Cc: Chenzhendong (alex) <alex.chen@huawei.com>; qemu-devel@nongnu.org;
>> qemu-trivial@nongnu.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
>> Chenqun (kuhn) <kuhn.chenqun@huawei.com>
>> Subject: [PATCH v2] json: Fix a memleak in parse_pair()
>> 
>> In qobject_type(), NULL is returned when the 'QObject' returned from
>> parse_value() is not of QString type, and this 'QObject' memory will leaked.
>> So we need to first cache the 'QObject' returned from parse_value(), and 
>> finally
>> free 'QObject' memory at the end of the function.
>> Also, we add a testcast about invalid dict key.
>> 
>> The memleak stack is as follows:
>> Direct leak of 32 byte(s) in 1 object(s) allocated from:
>>     #0 0xfffe4b3c34fb in __interceptor_malloc (/lib64/libasan.so.4+0xd34fb)
>>     #1 0xfffe4ae48aa3 in g_malloc (/lib64/libglib-2.0.so.0+0x58aa3)
>>     #2 0xaaab3557d9f7 in qnum_from_int
>> /Images/source_org/qemu_master/qemu/qobject/qnum.c:25
>>     #3 0xaaab35584d23 in parse_literal
>> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:511
>>     #4 0xaaab35584d23 in parse_value
>> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:554
>>     #5 0xaaab35583d77 in parse_pair
>> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:270
>>     #6 0xaaab355845db in parse_object
>> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:327
>>     #7 0xaaab355845db in parse_value
>> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:546
>>     #8 0xaaab35585b1b in json_parser_parse
>> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:580
>>     #9 0xaaab35583703 in json_message_process_token
>> /Images/source_org/qemu_master/qemu/qobject/json-streamer.c:92
>>     #10 0xaaab355ddccf in json_lexer_feed_char
>> /Images/source_org/qemu_master/qemu/qobject/json-lexer.c:313
>>     #11 0xaaab355de0eb in json_lexer_feed
>> /Images/source_org/qemu_master/qemu/qobject/json-lexer.c:350
>>     #12 0xaaab354aff67 in tcp_chr_read
>> /Images/source_org/qemu_master/qemu/chardev/char-socket.c:525
>>     #13 0xfffe4ae429db in g_main_context_dispatch
>> (/lib64/libglib-2.0.so.0+0x529db)
>>     #14 0xfffe4ae42d8f  (/lib64/libglib-2.0.so.0+0x52d8f)
>>     #15 0xfffe4ae430df in g_main_loop_run (/lib64/libglib-2.0.so.0+0x530df)
>>     #16 0xaaab34d70bff in iothread_run
>> /Images/source_org/qemu_master/qemu/iothread.c:82
>>     #17 0xaaab3559d71b in qemu_thread_start
>> /Images/source_org/qemu_master/qemu/util/qemu-thread-posix.c:519
>> 
>> Fixes: 532fb5328473 ("qapi: Make more of qobject_to()")
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qobject/json-parser.c | 12 ++++++------
>>  tests/check-qjson.c   |  9 +++++++++
>>  2 files changed, 15 insertions(+), 6 deletions(-)
>> 
>> diff --git a/qobject/json-parser.c b/qobject/json-parser.c index
>> d083810d37..c0f521b56b 100644
>> --- a/qobject/json-parser.c
>> +++ b/qobject/json-parser.c
>> @@ -257,8 +257,9 @@ static JSONToken
>> *parser_context_peek_token(JSONParserContext *ctxt)
>>   */
>>  static int parse_pair(JSONParserContext *ctxt, QDict *dict)  {
>> +    QObject *key_obj = NULL;
>> +    QString *key;
>>      QObject *value;
>> -    QString *key = NULL;
>>      JSONToken *peek, *token;
>> 
>>      peek = parser_context_peek_token(ctxt); @@ -267,7 +268,8 @@ static
>> int parse_pair(JSONParserContext *ctxt, QDict *dict)
>>          goto out;
>>      }
>> 
>> -    key = qobject_to(QString, parse_value(ctxt));
>> +    key_obj = parse_value(ctxt);
>> +    key = qobject_to(QString, key_obj);
>>      if (!key) {
>>          parse_error(ctxt, peek, "key is not a string in object");
>>          goto out;
>> @@ -297,13 +299,11 @@ static int parse_pair(JSONParserContext *ctxt,
>> QDict *dict)
>> 
>>      qdict_put_obj(dict, qstring_get_str(key), value);
>> 
>> -    qobject_unref(key);
>> -
>> +    qobject_unref(key_obj);
>>      return 0;
>> 
>>  out:
>> -    qobject_unref(key);
>> -
>> +    qobject_unref(key_obj);
>>      return -1;
>>  }
>> 
>
> Hi Alex,
>   Perhaps it would be more appropriate to provide the testcase as a separate 
> patch which from Markus.

I appreciate your consideration.  Since both fix and test case are
simple, and we need to hurry to have a chance at getting this into 5.2,
and the part of the work deserving credit the most is finding and
analyzing the bug, I'd like to take the patch as is.

Queued, thanks!