[Qemu-devel] [PATCH 0/4] Memory leak fixes

[Qemu-devel] [PATCH 0/4] Memory leak fixes

[Marc-André Lureau]

Hi,

A series of leaks spotted by ASAN. Some of them might be worth for 3.0...

Marc-André Lureau (4):
  tests: fix crumple/recursive leak
  tests: fix bdrv-drain leak
  monitor: fix oob command leak
  RFC: fix megasas leak

 hw/scsi/megasas.c         | 1 +
 monitor.c                 | 2 ++
 tests/check-block-qdict.c | 1 +
 tests/test-bdrv-drain.c   | 1 +
 4 files changed, 5 insertions(+)

-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 1/4] tests: fix crumple/recursive leak

[Marc-André Lureau]

Spotted by ASAN:

=================================================================
==27907==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 4120 byte(s) in 1 object(s) allocated from:
    #0 0x7f913458ce50 in calloc (/lib64/libasan.so.5+0xeee50)
    #1 0x7f9133fd641d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5241d)
    #2 0x5561c6643c95 in qdict_crumple_test_recursive /home/elmarco/src/qq/tests/check-block-qdict.c:438
    #3 0x7f9133ff7c49  (/lib64/libglib-2.0.so.0+0x73c49)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/check-block-qdict.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 478807f839..73d3e9f574 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -491,6 +491,7 @@ static void qdict_crumple_test_recursive(void)
     empty_list_0 = qobject_to(QDict, qlist_pop(empty_list));
     g_assert(empty_list_0);
     g_assert_cmpint(qdict_size(empty_list_0), ==, 0);
+    qobject_unref(empty_list_0);
 
     qobject_unref(src);
     qobject_unref(dst);
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 1/4] tests: fix crumple/recursive leak

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Spotted by ASAN:
>
> =================================================================
> ==27907==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 4120 byte(s) in 1 object(s) allocated from:
>     #0 0x7f913458ce50 in calloc (/lib64/libasan.so.5+0xeee50)
>     #1 0x7f9133fd641d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5241d)
>     #2 0x5561c6643c95 in qdict_crumple_test_recursive /home/elmarco/src/qq/tests/check-block-qdict.c:438
>     #3 0x7f9133ff7c49  (/lib64/libglib-2.0.so.0+0x73c49)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/check-block-qdict.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
> index 478807f839..73d3e9f574 100644
> --- a/tests/check-block-qdict.c
> +++ b/tests/check-block-qdict.c
> @@ -491,6 +491,7 @@ static void qdict_crumple_test_recursive(void)
>      empty_list_0 = qobject_to(QDict, qlist_pop(empty_list));
>      g_assert(empty_list_0);
>      g_assert_cmpint(qdict_size(empty_list_0), ==, 0);
> +    qobject_unref(empty_list_0);
>  
>      qobject_unref(src);
>      qobject_unref(dst);

Screwed up in commit 2860b2b2cb8.  I can add that to the commit message
when I apply.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 2/4] tests: fix bdrv-drain leak

[Marc-André Lureau]

Spotted by ASAN:

=================================================================
==5378==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 65536 byte(s) in 1 object(s) allocated from:
    #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
    #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
    #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
    #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-bdrv-drain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 17bb8508ae..abc8bbe6f0 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -948,6 +948,7 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
     }
 
     dbdd->done = true;
+    g_free(buffer);
 }
 
 /**
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 2/4] tests: fix bdrv-drain leak

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Spotted by ASAN:
>
> =================================================================
> ==5378==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 65536 byte(s) in 1 object(s) allocated from:
>     #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
>     #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>     #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
>     #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-bdrv-drain.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
> index 17bb8508ae..abc8bbe6f0 100644
> --- a/tests/test-bdrv-drain.c
> +++ b/tests/test-bdrv-drain.c
> @@ -948,6 +948,7 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
>      }
>  
>      dbdd->done = true;
> +    g_free(buffer);
>  }
>  
>  /**

Screwed up in commit 4c8158e359d.

Max, could the coroutine's stack accomodate the buffer?

Re: [Qemu-devel] [PATCH 2/4] tests: fix bdrv-drain leak

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]

On 2018-08-10 08:28, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
>> Spotted by ASAN:
>>
>> =================================================================
>> ==5378==ERROR: LeakSanitizer: detected memory leaks
>>
>> Direct leak of 65536 byte(s) in 1 object(s) allocated from:
>>     #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
>>     #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>>     #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
>>     #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  tests/test-bdrv-drain.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
>> index 17bb8508ae..abc8bbe6f0 100644
>> --- a/tests/test-bdrv-drain.c
>> +++ b/tests/test-bdrv-drain.c
>> @@ -948,6 +948,7 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
>>      }
>>  
>>      dbdd->done = true;
>> +    g_free(buffer);
>>  }
>>  
>>  /**
> 
> Screwed up in commit 4c8158e359d.
> 
> Max, could the coroutine's stack accomodate the buffer?

I don't know.  How big is a coroutine's stack?  The buffer size
apparently is 64k.  (Which is my opinion is generally too much to put on
any stack.)

But why would you want to put it on the stack anyway?  It's not like it
would make the patch any simpler.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/4] tests: fix bdrv-drain leak

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2018-08-10 08:28, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>> 
>>> Spotted by ASAN:
>>>
>>> =================================================================
>>> ==5378==ERROR: LeakSanitizer: detected memory leaks
>>>
>>> Direct leak of 65536 byte(s) in 1 object(s) allocated from:
>>>     #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
>>>     #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>>>     #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
>>>     #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  tests/test-bdrv-drain.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
>>> index 17bb8508ae..abc8bbe6f0 100644
>>> --- a/tests/test-bdrv-drain.c
>>> +++ b/tests/test-bdrv-drain.c
>>> @@ -948,6 +948,7 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
>>>      }
>>>  
>>>      dbdd->done = true;
>>> +    g_free(buffer);
>>>  }
>>>  
>>>  /**
>> 
>> Screwed up in commit 4c8158e359d.
>> 
>> Max, could the coroutine's stack accomodate the buffer?
>
> I don't know.  How big is a coroutine's stack?  The buffer size
> apparently is 64k.  (Which is my opinion is generally too much to put on
> any stack.)

Even in tests?

> But why would you want to put it on the stack anyway?  It's not like it
> would make the patch any simpler.

If you like the patch as is, go merge it as is :)

Re: [Qemu-devel] [PATCH 2/4] tests: fix bdrv-drain leak

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On 2018-08-13 08:15, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 2018-08-10 08:28, Markus Armbruster wrote:
>>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>>
>>>> Spotted by ASAN:
>>>>
>>>> =================================================================
>>>> ==5378==ERROR: LeakSanitizer: detected memory leaks
>>>>
>>>> Direct leak of 65536 byte(s) in 1 object(s) allocated from:
>>>>     #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
>>>>     #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>>>>     #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
>>>>     #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>>  tests/test-bdrv-drain.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
>>>> index 17bb8508ae..abc8bbe6f0 100644
>>>> --- a/tests/test-bdrv-drain.c
>>>> +++ b/tests/test-bdrv-drain.c
>>>> @@ -948,6 +948,7 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
>>>>      }
>>>>  
>>>>      dbdd->done = true;
>>>> +    g_free(buffer);
>>>>  }
>>>>  
>>>>  /**
>>>
>>> Screwed up in commit 4c8158e359d.
>>>
>>> Max, could the coroutine's stack accomodate the buffer?
>>
>> I don't know.  How big is a coroutine's stack?  The buffer size
>> apparently is 64k.  (Which is my opinion is generally too much to put on
>> any stack.)
> 
> Even in tests?

You have a point, but why do things differently in tests for no reason?

(So I don't leak memory, yeah, sure, but why not write the test in Rust
then from the start? O:-P)

>> But why would you want to put it on the stack anyway?  It's not like it
>> would make the patch any simpler.
> 
> If you like the patch as is, go merge it as is :)

W-well, I wasn't CC-d!

I take this as a request to take this one patch from the series and
merge it into my tree, so I will do that. O:-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/4] tests: fix bdrv-drain leak

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2018-08-13 08:15, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 2018-08-10 08:28, Markus Armbruster wrote:
>>>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>>>
>>>>> Spotted by ASAN:
>>>>>
>>>>> =================================================================
>>>>> ==5378==ERROR: LeakSanitizer: detected memory leaks
>>>>>
>>>>> Direct leak of 65536 byte(s) in 1 object(s) allocated from:
>>>>>     #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
>>>>>     #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>>>>>     #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
>>>>>     #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>  tests/test-bdrv-drain.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
>>>>> index 17bb8508ae..abc8bbe6f0 100644
>>>>> --- a/tests/test-bdrv-drain.c
>>>>> +++ b/tests/test-bdrv-drain.c
>>>>> @@ -948,6 +948,7 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
>>>>>      }
>>>>>  
>>>>>      dbdd->done = true;
>>>>> +    g_free(buffer);
>>>>>  }
>>>>>  
>>>>>  /**
>>>>
>>>> Screwed up in commit 4c8158e359d.
>>>>
>>>> Max, could the coroutine's stack accomodate the buffer?
>>>
>>> I don't know.  How big is a coroutine's stack?  The buffer size
>>> apparently is 64k.  (Which is my opinion is generally too much to put on
>>> any stack.)
>> 
>> Even in tests?
>
> You have a point, but why do things differently in tests for no reason?
>
> (So I don't leak memory, yeah, sure, but why not write the test in Rust
> then from the start? O:-P)

Only 230 days to April 1st, better hurry.

>>> But why would you want to put it on the stack anyway?  It's not like it
>>> would make the patch any simpler.
>> 
>> If you like the patch as is, go merge it as is :)
>
> W-well, I wasn't CC-d!

I fixed that :)

> I take this as a request to take this one patch from the series and
> merge it into my tree, so I will do that. O:-)

Thanks!

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] tests: fix bdrv-drain leak

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On 2018-08-09 13:44, Marc-André Lureau wrote:
> Spotted by ASAN:
> 
> =================================================================
> ==5378==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 65536 byte(s) in 1 object(s) allocated from:
>     #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
>     #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>     #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
>     #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-bdrv-drain.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
> index 17bb8508ae..abc8bbe6f0 100644
> --- a/tests/test-bdrv-drain.c
> +++ b/tests/test-bdrv-drain.c
> @@ -948,6 +948,7 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
>      }
>  
>      dbdd->done = true;
> +    g_free(buffer);
>  }
>  
>  /**
> 

Thanks, added a note of the commit that broke it to the commit message
and applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 3/4] monitor: fix oob command leak

[Marc-André Lureau]

Spotted by ASAN, during make check...

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x7f8e27262c48 in malloc (/lib64/libasan.so.5+0xeec48)
    #1 0x7f8e26a5f3c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
    #2 0x555ab67078a8 in qstring_from_str /home/elmarco/src/qq/qobject/qstring.c:67
    #3 0x555ab67071e4 in qstring_new /home/elmarco/src/qq/qobject/qstring.c:24
    #4 0x555ab6713fbf in qstring_from_escaped_str /home/elmarco/src/qq/qobject/json-parser.c:144
    #5 0x555ab671738c in parse_literal /home/elmarco/src/qq/qobject/json-parser.c:506
    #6 0x555ab67179c3 in parse_value /home/elmarco/src/qq/qobject/json-parser.c:569
    #7 0x555ab6715123 in parse_pair /home/elmarco/src/qq/qobject/json-parser.c:306
    #8 0x555ab6715483 in parse_object /home/elmarco/src/qq/qobject/json-parser.c:357
    #9 0x555ab671798b in parse_value /home/elmarco/src/qq/qobject/json-parser.c:561
    #10 0x555ab6717a6b in json_parser_parse_err /home/elmarco/src/qq/qobject/json-parser.c:592
    #11 0x555ab4fd4dcf in handle_qmp_command /home/elmarco/src/qq/monitor.c:4257
    #12 0x555ab6712c4d in json_message_process_token /home/elmarco/src/qq/qobject/json-streamer.c:105
    #13 0x555ab67e01e2 in json_lexer_feed_char /home/elmarco/src/qq/qobject/json-lexer.c:323
    #14 0x555ab67e0af6 in json_lexer_feed /home/elmarco/src/qq/qobject/json-lexer.c:373
    #15 0x555ab6713010 in json_message_parser_feed /home/elmarco/src/qq/qobject/json-streamer.c:124
    #16 0x555ab4fd58ec in monitor_qmp_read /home/elmarco/src/qq/monitor.c:4337
    #17 0x555ab6559df2 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:175
    #18 0x555ab6559e95 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:187
    #19 0x555ab6560127 in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
    #20 0x555ab65d9c73 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
    #21 0x7f8e26a598ac in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x4c8ac)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/monitor.c b/monitor.c
index 77861e96af..a1999e396c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4277,6 +4277,8 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id)
                                           ?: "");
         monitor_qmp_dispatch(mon, req, id);
+        qobject_unref(req);
+        qobject_unref(id);
         return;
     }
 
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 3/4] monitor: fix oob command leak

[Marc-André Lureau]

Hi

On Thu, Aug 9, 2018 at 1:44 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Spotted by ASAN, during make check...
>
> Direct leak of 40 byte(s) in 1 object(s) allocated from:
>     #0 0x7f8e27262c48 in malloc (/lib64/libasan.so.5+0xeec48)
>     #1 0x7f8e26a5f3c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>     #2 0x555ab67078a8 in qstring_from_str /home/elmarco/src/qq/qobject/qstring.c:67
>     #3 0x555ab67071e4 in qstring_new /home/elmarco/src/qq/qobject/qstring.c:24
>     #4 0x555ab6713fbf in qstring_from_escaped_str /home/elmarco/src/qq/qobject/json-parser.c:144
>     #5 0x555ab671738c in parse_literal /home/elmarco/src/qq/qobject/json-parser.c:506
>     #6 0x555ab67179c3 in parse_value /home/elmarco/src/qq/qobject/json-parser.c:569
>     #7 0x555ab6715123 in parse_pair /home/elmarco/src/qq/qobject/json-parser.c:306
>     #8 0x555ab6715483 in parse_object /home/elmarco/src/qq/qobject/json-parser.c:357
>     #9 0x555ab671798b in parse_value /home/elmarco/src/qq/qobject/json-parser.c:561
>     #10 0x555ab6717a6b in json_parser_parse_err /home/elmarco/src/qq/qobject/json-parser.c:592
>     #11 0x555ab4fd4dcf in handle_qmp_command /home/elmarco/src/qq/monitor.c:4257
>     #12 0x555ab6712c4d in json_message_process_token /home/elmarco/src/qq/qobject/json-streamer.c:105
>     #13 0x555ab67e01e2 in json_lexer_feed_char /home/elmarco/src/qq/qobject/json-lexer.c:323
>     #14 0x555ab67e0af6 in json_lexer_feed /home/elmarco/src/qq/qobject/json-lexer.c:373
>     #15 0x555ab6713010 in json_message_parser_feed /home/elmarco/src/qq/qobject/json-streamer.c:124
>     #16 0x555ab4fd58ec in monitor_qmp_read /home/elmarco/src/qq/monitor.c:4337
>     #17 0x555ab6559df2 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:175
>     #18 0x555ab6559e95 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:187
>     #19 0x555ab6560127 in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
>     #20 0x555ab65d9c73 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
>     #21 0x7f8e26a598ac in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x4c8ac)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

It looks like it was introduced with commit
b27314567d4cd8e204a18feba60d3341fb2d1aed "qmp: Simplify code around
monitor_qmp_dispatch_one()"

> ---
>  monitor.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index 77861e96af..a1999e396c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4277,6 +4277,8 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id)
>                                            ?: "");
>          monitor_qmp_dispatch(mon, req, id);
> +        qobject_unref(req);
> +        qobject_unref(id);
>          return;
>      }
>
> --
> 2.18.0.547.g1d89318c48
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 3/4] monitor: fix oob command leak

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Aug 9, 2018 at 1:44 PM, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>> Spotted by ASAN, during make check...
>>
>> Direct leak of 40 byte(s) in 1 object(s) allocated from:
>>     #0 0x7f8e27262c48 in malloc (/lib64/libasan.so.5+0xeec48)
>>     #1 0x7f8e26a5f3c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
>>     #2 0x555ab67078a8 in qstring_from_str /home/elmarco/src/qq/qobject/qstring.c:67
>>     #3 0x555ab67071e4 in qstring_new /home/elmarco/src/qq/qobject/qstring.c:24
>>     #4 0x555ab6713fbf in qstring_from_escaped_str /home/elmarco/src/qq/qobject/json-parser.c:144
>>     #5 0x555ab671738c in parse_literal /home/elmarco/src/qq/qobject/json-parser.c:506
>>     #6 0x555ab67179c3 in parse_value /home/elmarco/src/qq/qobject/json-parser.c:569
>>     #7 0x555ab6715123 in parse_pair /home/elmarco/src/qq/qobject/json-parser.c:306
>>     #8 0x555ab6715483 in parse_object /home/elmarco/src/qq/qobject/json-parser.c:357
>>     #9 0x555ab671798b in parse_value /home/elmarco/src/qq/qobject/json-parser.c:561
>>     #10 0x555ab6717a6b in json_parser_parse_err /home/elmarco/src/qq/qobject/json-parser.c:592
>>     #11 0x555ab4fd4dcf in handle_qmp_command /home/elmarco/src/qq/monitor.c:4257
>>     #12 0x555ab6712c4d in json_message_process_token /home/elmarco/src/qq/qobject/json-streamer.c:105
>>     #13 0x555ab67e01e2 in json_lexer_feed_char /home/elmarco/src/qq/qobject/json-lexer.c:323
>>     #14 0x555ab67e0af6 in json_lexer_feed /home/elmarco/src/qq/qobject/json-lexer.c:373
>>     #15 0x555ab6713010 in json_message_parser_feed /home/elmarco/src/qq/qobject/json-streamer.c:124
>>     #16 0x555ab4fd58ec in monitor_qmp_read /home/elmarco/src/qq/monitor.c:4337
>>     #17 0x555ab6559df2 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:175
>>     #18 0x555ab6559e95 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:187
>>     #19 0x555ab6560127 in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
>>     #20 0x555ab65d9c73 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
>>     #21 0x7f8e26a598ac in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x4c8ac)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> It looks like it was introduced with commit
> b27314567d4cd8e204a18feba60d3341fb2d1aed "qmp: Simplify code around
> monitor_qmp_dispatch_one()"

Yes.

Before that commit, handle_qmp_command() freed @req and @id as follows.

* Early errors: goto err, where we free req, and leak @id (I think).

* Out of band: store @req and @id in @req_obj, free it in
  monitor_qmp_dispatch_one().

* In band: store @req and @id in @req_obj

  - queue full: free by calling qmp_request_free()

  - else: enqueue, monitor_qmp_bh_dispatcher() will deqeueue and pass to
    monitor_qmp_dispatch_one(), which frees.

Current head always stores @req and @id in @req_obj.  It frees as
follows.

* Out of band: it doesn't, since monitor_qmp_dispatch(), renamed from
  monitor_qmp_dispatch_one(), doesn't free anymore.  This patch fixes
  it.

* In band:

  - queue full: free by calling qmp_request_free().

  - else: enqueue, monitor_qmp_bh_dispatcher() will deqeueue and pass to
    monitor_qmp_dispatch(), then free with qmp_request_free().

Looks like the commit replaced one leak by another one.

>
>> ---
>>  monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 77861e96af..a1999e396c 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4277,6 +4277,8 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>>          trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id)
>>                                            ?: "");
>>          monitor_qmp_dispatch(mon, req, id);
>> +        qobject_unref(req);
>> +        qobject_unref(id);
>>          return;
>>      }
>>
>> --
>> 2.18.0.547.g1d89318c48
>>
>>

I'm not going to argue for inclusion in 3.0, because OOB is experimental
and off by default due to its remaining flaws.

Cc: qemu-stable@nongnu.org
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 4/4] RFC: fix megasas leak

[Marc-André Lureau]

tests/cdrom-test -p /x86_64/cdrom/boot/megasas

Produces the following ASAN leak.

==25700==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7f06f8faac48 in malloc (/lib64/libasan.so.5+0xeec48)
    #1 0x7f06f87a73c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
    #2 0x55a729f17738 in pci_dma_sglist_init /home/elmarco/src/qq/include/hw/pci/pci.h:818
    #3 0x55a729f2a706 in megasas_map_dcmd /home/elmarco/src/qq/hw/scsi/megasas.c:698
    #4 0x55a729f39421 in megasas_handle_dcmd /home/elmarco/src/qq/hw/scsi/megasas.c:1574
    #5 0x55a729f3f70d in megasas_handle_frame /home/elmarco/src/qq/hw/scsi/megasas.c:1955
    #6 0x55a729f40939 in megasas_mmio_write /home/elmarco/src/qq/hw/scsi/megasas.c:2119
    #7 0x55a729f41102 in megasas_port_write /home/elmarco/src/qq/hw/scsi/megasas.c:2170
    #8 0x55a729220e60 in memory_region_write_accessor /home/elmarco/src/qq/memory.c:527
    #9 0x55a7292212b3 in access_with_adjusted_size /home/elmarco/src/qq/memory.c:594
    #10 0x55a72922cf70 in memory_region_dispatch_write /home/elmarco/src/qq/memory.c:1473
    #11 0x55a7290f5907 in flatview_write_continue /home/elmarco/src/qq/exec.c:3255
    #12 0x55a7290f5ceb in flatview_write /home/elmarco/src/qq/exec.c:3294
    #13 0x55a7290f6457 in address_space_write /home/elmarco/src/qq/exec.c:3384
    #14 0x55a7290f64a8 in address_space_rw /home/elmarco/src/qq/exec.c:3395
    #15 0x55a72929ecb0 in kvm_handle_io /home/elmarco/src/qq/accel/kvm/kvm-all.c:1729
    #16 0x55a7292a0db5 in kvm_cpu_exec /home/elmarco/src/qq/accel/kvm/kvm-all.c:1969
    #17 0x55a7291c4212 in qemu_kvm_cpu_thread_fn /home/elmarco/src/qq/cpus.c:1215
    #18 0x55a72a966a6c in qemu_thread_start /home/elmarco/src/qq/util/qemu-thread-posix.c:504
    #19 0x7f06ed486593 in start_thread (/lib64/libpthread.so.0+0x7593)

I suppose megasas_complete_command() should take care of destroying
the sglist instead, so I leave that patch as RFC for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/scsi/megasas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index ba1afa3c1e..2a16397b3b 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -465,6 +465,7 @@ static void megasas_unmap_frame(MegasasState *s, MegasasCmd *cmd)
     cmd->pa = 0;
     cmd->pa_size = 0;
     clear_bit(cmd->index, s->frame_map);
+    qemu_sglist_destroy(&cmd->qsg);
 }
 
 /*
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 4/4] RFC: fix megasas leak

[Paolo Bonzini]

On 09/08/2018 13:44, Marc-André Lureau wrote:
> 
> I suppose megasas_complete_command() should take care of destroying
> the sglist instead, so I leave that patch as RFC for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

megasas_complete_command() is already doing it, and it calls
megasas_unmap_frame, so it is okay to remove the qemu_sglist_destroy
from there and move it to megasas_unmap_frame.  Thanks!

Paolo

> ---
>  hw/scsi/megasas.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index ba1afa3c1e..2a16397b3b 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -465,6 +465,7 @@ static void megasas_unmap_frame(MegasasState *s, MegasasCmd *cmd)
>      cmd->pa = 0;
>      cmd->pa_size = 0;
>      clear_bit(cmd->index, s->frame_map);
> +    qemu_sglist_destroy(&cmd->qsg);
>  }
>  
>  /*
> -- 2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 0/4] Memory leak fixes

[Eric Blake]

On 08/09/2018 06:44 AM, Marc-André Lureau wrote:
> Hi,
> 
> A series of leaks spotted by ASAN. Some of them might be worth for 3.0...

Too late for 3.0.0, but you can cc qemu-stable for 3.0.1.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org