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

Spotted by ASAN:

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?

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

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 :)

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

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

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!