Re: [Qemu-stable] [Qemu-devel] [PATCH] json-streamer: Don't leak tokens on incomplete parse

[Markus Armbruster]

Changlong Xie <address@hidden> writes:

> On 05/19/2016 05:46 AM, Eric Blake wrote:
>> Valgrind complained about a number of leaks in
>> tests/check-qobject-json:
>>
>> ==12657==    definitely lost: 17,247 bytes in 1,234 blocks
>>
>> All of which had the same root cause: on an incomplete parse,
>> we were abandoning the token queue without cleaning up the
>> allocated data within each queue element.  Introduced in
>> commit 95385fe, when we switched from QList (which recursively
>> frees contents) to g_queue (which does not).
>>
>> We don't yet require glib 2.32 with its g_queue_free_full(),
>> so open-code it instead.
>>
>> CC: address@hidden
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>   qobject/json-streamer.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
>> index 0251685..7164390 100644
>> --- a/qobject/json-streamer.c
>> +++ b/qobject/json-streamer.c
>> @@ -20,9 +20,15 @@
>>   #define MAX_TOKEN_COUNT (2ULL << 20)
>>   #define MAX_NESTING (1ULL << 10)
>>
>> +static void json_message_free_token(void *token, void *opaque)
>> +{
>> +    g_free(token);
>> +}
>> +
>>   static void json_message_free_tokens(JSONMessageParser *parser)
>>   {
>>       if (parser->tokens) {
>> +        g_queue_foreach(parser->tokens, json_message_free_token, NULL);
>>           g_queue_free(parser->tokens);
>>           parser->tokens = NULL;
>>       }
>>
>
> It seems this commit make tests/qemu-iotests/071 failed on the last
> master branch.

More direct reproducer:

    { "execute": "qmp_capabilities" }
    { "execute": "blockdev-add",
        "arguments": {
            "options": {
                "node-name": "drive0",
                "driver": "qcow2",
                "file": {
                    "driver": "file",
                    "filename": "t.qcow2"
                }
            }
        }
    }
    { "execute": "blockdev-add",
        "arguments": {
            "options": {
                "driver": "blkverify",
                "id": "drive0-verify",
                "test": "drive0",
                "raw": {
                    "driver": "file",
                    "filename": "t.qcow2.base"
                }
            }
        }
    }
    { "execute": "human-monitor-command",
        "arguments": {
            "command-line": 'qemu-io drive0-verify "read 0 512"'
        }
    }

With t.qcow2 and t.qcow2.base as in 071.

Interesting part of valgrind run:

    ==29716==    at 0xF7E8E71: g_queue_foreach (in 
/usr/lib64/libglib-2.0.so.0.4600.2)
    ==29716==    by 0x7CB7C5: json_message_free_tokens (json-streamer.c:31)
    ==29716==    by 0x7CBAC6: json_message_parser_destroy (json-streamer.c:131)
    ==29716==    by 0x3469AE: monitor_qmp_event (monitor.c:4022)
    ==29716==    by 0x476C32: qemu_chr_be_event (qemu-char.c:205)
    ==29716==    by 0x47BE05: tcp_chr_close (qemu-char.c:3175)
    ==29716==    by 0x47E5AB: qemu_chr_free (qemu-char.c:4036)
    ==29716==    by 0x47E62C: qemu_chr_delete (qemu-char.c:4044)
    ==29716==    by 0x47F576: qemu_chr_cleanup (qemu-char.c:4557)
    ==29716==    by 0x12EA65E7: __run_exit_handlers (in /usr/lib64/libc-2.22.so)
    ==29716==    by 0x12EA6634: exit (in /usr/lib64/libc-2.22.so)
    ==29716==    by 0x74762C: blkverify_err (blkverify.c:58)
    ==29716==  Address 0x24270550 is 0 bytes inside a block of size 24 free'd
    ==29716==    at 0x4C29CF0: free (vg_replace_malloc.c:530)
    ==29716==    by 0xF7DE63D: g_free (in /usr/lib64/libglib-2.0.so.0.4600.2)
    ==29716==    by 0xF7F5DCC: g_slice_free1 (in 
/usr/lib64/libglib-2.0.so.0.4600.2)
    ==29716==    by 0x7CC327: parser_context_free (json-parser.c:268)
    ==29716==    by 0x7CCFB5: json_parser_parse_err (json-parser.c:577)
    ==29716==    by 0x7CCF48: json_parser_parse (json-parser.c:561)
    ==29716==    by 0x3464A2: handle_qmp_command (monitor.c:3892)
    ==29716==    by 0x7CB9C7: json_message_process_token (json-streamer.c:100)
    ==29716==    by 0x7EDEC2: json_lexer_feed_char (json-lexer.c:319)
    ==29716==    by 0x7EE00A: json_lexer_feed (json-lexer.c:369)
    ==29716==    by 0x7CBA7E: json_message_parser_feed (json-streamer.c:120)
    ==29716==    by 0x346722: monitor_qmp_read (monitor.c:3949)
    ==29716==  Block was alloc'd at
    ==29716==    at 0x4C28BF6: malloc (vg_replace_malloc.c:299)
    ==29716==    by 0xF7DE528: g_malloc (in /usr/lib64/libglib-2.0.so.0.4600.2)
    ==29716==    by 0xF7F5652: g_slice_alloc (in 
/usr/lib64/libglib-2.0.so.0.4600.2)
    ==29716==    by 0xF7F5CED: g_slice_alloc0 (in 
/usr/lib64/libglib-2.0.so.0.4600.2)
    ==29716==    by 0x7CB9CC: json_message_process_token (json-streamer.c:101)
    ==29716==    by 0x7EDEC2: json_lexer_feed_char (json-lexer.c:319)
    ==29716==    by 0x7EE00A: json_lexer_feed (json-lexer.c:369)
    ==29716==    by 0x7CBA7E: json_message_parser_feed (json-streamer.c:120)
    ==29716==    by 0x346722: monitor_qmp_read (monitor.c:3949)
    ==29716==    by 0x477214: qemu_chr_be_write_impl (qemu-char.c:388)
    ==29716==    by 0x477272: qemu_chr_be_write (qemu-char.c:400)
    ==29716==    by 0x47B4CD: tcp_chr_read (qemu-char.c:2894)

Double free.  Can't see offhand how this stuff works.  Eric, let's
revert this patch unless you can see a fix.