diff mbox series

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

Message ID 20201113145525.85151-1-alex.chen@huawei.com (mailing list archive)
State New, archived
Headers show
Series [v2] json: Fix a memleak in parse_pair() | expand

Commit Message

Alex Chen Nov. 13, 2020, 2:55 p.m. UTC
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(-)

Comments

Chenqun (kuhn) Nov. 14, 2020, 9:08 a.m. UTC | #1
> -----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.

Thanks,
Chen Qun
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c index
> 07a773e653..9a02079099 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -1415,6 +1415,14 @@ static void invalid_dict_comma(void)
>      g_assert(obj == NULL);
>  }
> 
> +static void invalid_dict_key(void)
> +{
> +    Error *err = NULL;
> +    QObject *obj = qobject_from_json("{32:'abc'}", &err);
> +    error_free_or_abort(&err);
> +    g_assert(obj == NULL);
> +}
> +
>  static void unterminated_literal(void)
>  {
>      Error *err = NULL;
> @@ -1500,6 +1508,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/errors/unterminated/dict_comma",
> unterminated_dict_comma);
>      g_test_add_func("/errors/invalid_array_comma",
> invalid_array_comma);
>      g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
> +    g_test_add_func("/errors/invalid_dict_key", invalid_dict_key);
>      g_test_add_func("/errors/unterminated/literal", unterminated_literal);
>      g_test_add_func("/errors/limits/nesting", limits_nesting);
>      g_test_add_func("/errors/multiple_values", multiple_values);
> --
> 2.19.1
Markus Armbruster Nov. 16, 2020, 6:42 a.m. UTC | #2
"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!
Philippe Mathieu-Daudé Nov. 16, 2020, 11:43 a.m. UTC | #3
On 11/16/20 7:42 AM, Markus Armbruster wrote:
> "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
>>>
...
> 
> Queued, thanks!

If possible can you s%/Images/source_org/qemu_master/qemu/%% to make
description more readable...?

Thanks,

Phil.
Alex Chen Nov. 16, 2020, 1:22 p.m. UTC | #4
On 2020/11/16 19:43, Philippe Mathieu-Daudé wrote:
> On 11/16/20 7:42 AM, Markus Armbruster wrote:
>> "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
>>>>
> ...
>>
>> Queued, thanks!
> 
> If possible can you s%/Images/source_org/qemu_master/qemu/%% to make
> description more readable...?
> 

Hi Philippe,
I am sorry for that, considering that the patch has been queued,
do I need to modify the commit message and send patch v3?

Thanks,
Alex
Markus Armbruster Nov. 16, 2020, 2:03 p.m. UTC | #5
Alex Chen <alex.chen@huawei.com> writes:

> On 2020/11/16 19:43, Philippe Mathieu-Daudé wrote:
>> On 11/16/20 7:42 AM, Markus Armbruster wrote:
>>> "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
>>>>>
>> ...
>>>
>>> Queued, thanks!
>> 
>> If possible can you s%/Images/source_org/qemu_master/qemu/%% to make
>> description more readable...?
>> 
>
> Hi Philippe,
> I am sorry for that, considering that the patch has been queued,
> do I need to modify the commit message and send patch v3?

I'll take care of it, no need to respin.  Thanks!
Philippe Mathieu-Daudé Nov. 16, 2020, 2:06 p.m. UTC | #6
On 11/16/20 3:03 PM, Markus Armbruster wrote:
> Alex Chen <alex.chen@huawei.com> writes:
> 
>> On 2020/11/16 19:43, Philippe Mathieu-Daudé wrote:
>>> On 11/16/20 7:42 AM, Markus Armbruster wrote:
>>>> "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
>>>>>>
>>> ...
>>>>
>>>> Queued, thanks!
>>>
>>> If possible can you s%/Images/source_org/qemu_master/qemu/%% to make
>>> description more readable...?
>>>
>>
>> Hi Philippe,
>> I am sorry for that, considering that the patch has been queued,
>> do I need to modify the commit message and send patch v3?

As the patch was already queued, this was a comment for the maintainer,
sorry for not being clear enough.

> 
> I'll take care of it, no need to respin.  Thanks!

Thanks!
diff mbox series

Patch

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;
 }
 
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 07a773e653..9a02079099 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1415,6 +1415,14 @@  static void invalid_dict_comma(void)
     g_assert(obj == NULL);
 }
 
+static void invalid_dict_key(void)
+{
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("{32:'abc'}", &err);
+    error_free_or_abort(&err);
+    g_assert(obj == NULL);
+}
+
 static void unterminated_literal(void)
 {
     Error *err = NULL;
@@ -1500,6 +1508,7 @@  int main(int argc, char **argv)
     g_test_add_func("/errors/unterminated/dict_comma", unterminated_dict_comma);
     g_test_add_func("/errors/invalid_array_comma", invalid_array_comma);
     g_test_add_func("/errors/invalid_dict_comma", invalid_dict_comma);
+    g_test_add_func("/errors/invalid_dict_key", invalid_dict_key);
     g_test_add_func("/errors/unterminated/literal", unterminated_literal);
     g_test_add_func("/errors/limits/nesting", limits_nesting);
     g_test_add_func("/errors/multiple_values", multiple_values);