* [PATCH] json: Fix a memleak in parse_pair()
@ 2020-11-11 11:56 Alex Chen
2020-11-12 6:37 ` Markus Armbruster
0 siblings, 1 reply; 3+ messages in thread
From: Alex Chen @ 2020-11-11 11:56 UTC (permalink / raw)
To: armbru; +Cc: alex.chen, qemu-trivial, qemu-devel, zhang.zhanghailiang
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 check whether the 'QObject' returned from parse_value() is of QString type,
and if not, we free 'QObject' memory and return an error.
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>
---
qobject/json-parser.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index d083810d37..b37762a203 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -267,10 +267,13 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict)
goto out;
}
- key = qobject_to(QString, parse_value(ctxt));
- if (!key) {
- parse_error(ctxt, peek, "key is not a string in object");
+ value = parse_value(ctxt);
+ if (!value || qobject_type(value) != QTYPE_QSTRING) {
+ qobject_unref(value);
+ parse_error(ctxt, peek, "value is not a string in object");
goto out;
+ } else {
+ key = qobject_to(QString, value);
}
token = parser_context_pop_token(ctxt);
--
2.19.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] json: Fix a memleak in parse_pair() 2020-11-11 11:56 [PATCH] json: Fix a memleak in parse_pair() Alex Chen @ 2020-11-12 6:37 ` Markus Armbruster 2020-11-13 14:51 ` Alex Chen 0 siblings, 1 reply; 3+ messages in thread From: Markus Armbruster @ 2020-11-12 6:37 UTC (permalink / raw) To: Alex Chen; +Cc: qemu-trivial, qemu-devel, zhang.zhanghailiang Alex Chen <alex.chen@huawei.com> writes: > 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 check whether the 'QObject' returned from parse_value() is of QString type, > and if not, we free 'QObject' memory and return an error. > > 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 Good catch, thanks! > 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> > --- > qobject/json-parser.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/qobject/json-parser.c b/qobject/json-parser.c > index d083810d37..b37762a203 100644 > --- a/qobject/json-parser.c > +++ b/qobject/json-parser.c > @@ -267,10 +267,13 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict) > goto out; > } > > - key = qobject_to(QString, parse_value(ctxt)); > - if (!key) { > - parse_error(ctxt, peek, "key is not a string in object"); > + value = parse_value(ctxt); > + if (!value || qobject_type(value) != QTYPE_QSTRING) { The "is string" test becomes more cumbersome. Also, we reuse @value for another purpose. > + qobject_unref(value); > + parse_error(ctxt, peek, "value is not a string in object"); Changing error messages in a memory leak fix is inappropriate. Accident? > goto out; > + } else { > + key = qobject_to(QString, value); > } > > token = parser_context_pop_token(ctxt); Here's a simpler, straightforward fix, with a test case thrown in: 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); ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] json: Fix a memleak in parse_pair() 2020-11-12 6:37 ` Markus Armbruster @ 2020-11-13 14:51 ` Alex Chen 0 siblings, 0 replies; 3+ messages in thread From: Alex Chen @ 2020-11-13 14:51 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-trivial, qemu-devel, zhang.zhanghailiang On 2020/11/12 14:37, Markus Armbruster wrote: > Alex Chen <alex.chen@huawei.com> writes: > >> 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 check whether the 'QObject' returned from parse_value() is of QString type, >> and if not, we free 'QObject' memory and return an error. >> >> 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 > > Good catch, thanks! > >> 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> >> --- >> qobject/json-parser.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/qobject/json-parser.c b/qobject/json-parser.c >> index d083810d37..b37762a203 100644 >> --- a/qobject/json-parser.c >> +++ b/qobject/json-parser.c >> @@ -267,10 +267,13 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict) >> goto out; >> } >> >> - key = qobject_to(QString, parse_value(ctxt)); >> - if (!key) { >> - parse_error(ctxt, peek, "key is not a string in object"); >> + value = parse_value(ctxt); >> + if (!value || qobject_type(value) != QTYPE_QSTRING) { > > The "is string" test becomes more cumbersome. Also, we reuse @value for > another purpose. > >> + qobject_unref(value); >> + parse_error(ctxt, peek, "value is not a string in object"); > > Changing error messages in a memory leak fix is inappropriate. > Accident? > >> goto out; >> + } else { >> + key = qobject_to(QString, value); >> } >> >> token = parser_context_pop_token(ctxt); > > Here's a simpler, straightforward fix, with a test case thrown in: > > 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); > Thanks for your review，your patch is better than mine. I will send the patch v2. Thanks, Alex ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-13 14:53 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-11-11 11:56 [PATCH] json: Fix a memleak in parse_pair() Alex Chen 2020-11-12 6:37 ` Markus Armbruster 2020-11-13 14:51 ` Alex Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).