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

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

[Eric Blake]

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: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 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;
     }
-- 
2.5.5

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> 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.

Should we add a replacement g_queue_free_full() to glib-compat.h?

> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  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;
>      }

Since open-coding is a one-liner, I'm okay with it.

Applied to my qapi-next branch, thanks!

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

[Changlong Xie]

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: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   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.

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

[Markus Armbruster]

Changlong Xie <xiecl.fnst@cn.fujitsu.com> 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: qemu-stable@nongnu.org
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   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.

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

[Paolo Bonzini]

On 04/07/2016 14:21, Markus Armbruster wrote:
> Double free.  Can't see offhand how this stuff works.  Eric, let's
> revert this patch unless you can see a fix.

I think I have a fix.

Paolo