Re: [Qemu-devel] [PATCH v9 03/27] qapi: Plug leaks in test-qmp-*

[Eric Blake <eblake@redhat.com>]

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

On 11/04/2015 01:19 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Make valgrind happy with the current state of the tests, so that
>> it is easier to see if future patches introduce new memory problems
>> without being drowned in noise.  Many of the leaks were due to
>> calling a second init without tearing down the data from an earlier
>> visit.  But since teardown is already idempotent, and we already
>> register teardown as part of input_visitor_test_add(), it is nicer
>> to just make init() safe to call multiple times than it is to have
>> to make all tests call teardown.
>>
>> Another common leak was forgetting to clean up an error object,
>> after testing that an error was raised.
>>
>> Another leak was in test_visitor_in_struct_nested(), failing to
>> clean the base member of UserDefTwo.  Cleaning that up left
>> check_and_free_str() as dead code (since using the qapi_free_*
>> takes care of recursion, and we don't want double frees).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v9: move earlier in series (was 13/17)
>> v8: no change
>> v7: no change
>> v6: make init repeatable rather than adding teardown everywhere,
>> fix additional leak with UserDefTwo base, plug additional files
>> ---
>>  tests/test-qmp-input-strict.c   | 10 ++++++++++
>>  tests/test-qmp-input-visitor.c  | 41 +++++++----------------------------------
>>  tests/test-qmp-output-visitor.c |  3 ++-
>>  3 files changed, 19 insertions(+), 35 deletions(-)
> 
> No leaks to plug in test/-qmp-commands.c and test-qmp-event.c?

Didn't check. I'll do that.

> 
>> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
>> index b44184f..910e2f9 100644
>> --- a/tests/test-qmp-input-strict.c
>> +++ b/tests/test-qmp-input-strict.c
>> @@ -77,6 +77,8 @@ static Visitor *validate_test_init_raw(TestInputVisitorData *data,
>>  {
>>      Visitor *v;
>>
>> +    validate_teardown(data, NULL);
>> +
>>      data->obj = qobject_from_json(json_string);
>>      g_assert(data->obj != NULL);
>>
> 
> A test added with validate_test_add() may call validate_test_init_raw()
> any number of time.  Since validate_test_add() passes
> validate_teardown() as fixture teardown function, the last one will be
> cleaned up on test finalization.  The others will be cleaned up by the
> next validate_test_init_raw().  Okay.  Actually, the whole fixture
> business starts to make sense only now.
> 
> But why only validate_test_init_raw() and not validate_test_init()?
> 

Umm, because I didn't look, and just plugged holes? Will fix.

> The two duplicate code, by the way.

And the fix will probably be by having one call the other.

> 
>> @@ -193,6 +195,8 @@ static void test_validate_fail_struct(TestInputVisitorData *data,
>>
>>      visit_type_TestStruct(v, &p, NULL, &err);
>>      g_assert(err);
>> +    error_free(err);
>> +    /* FIXME: visitor should not allocate p when returning error */
> 
> Indeed.
> 
> Recommend to always mention new FIXMEs in the commit message.

This fixme is part of the answer to your question about 6/27 - we do
have test coverage on non-arrays.


>> +++ b/tests/test-qmp-input-visitor.c
>> @@ -46,6 +46,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
>>      Visitor *v;
>>      va_list ap;
>>
>> +    visitor_input_teardown(data, NULL);
>> +
>>      va_start(ap, json_string);
>>      data->obj = qobject_from_jsonv(json_string, &ap);
>>      va_end(ap);
> 
> Here, you add it to visitor_input_test_init(), but not
> visitor_input_test_init_raw().
> 
> These two duplicate code, too.

Looks like I get to fix this too.


>> +++ b/tests/test-qmp-output-visitor.c
>> @@ -391,6 +391,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
>>      qobj = QOBJECT(qdict);

Here, qobj is an alias to qdict...

>>      visit_type_any(data->ov, &qobj, NULL, &err);
>>      g_assert(!err);
>> +    qobject_decref(qobj);
>>      obj = qmp_output_get_qobject(data->qov);
>>      g_assert(obj != NULL);
>>      qdict = qobject_to_qdict(obj);
>> @@ -411,7 +412,6 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
>        qobj = qdict_get(qdict, "string");

...but then we are reassigning it to instead be an alias within qdict.

>        g_assert(qobj);
>        qstring = qobject_to_qstring(qobj);
>>      g_assert(qstring);
>>      g_assert_cmpstr(qstring_get_str(qstring), ==, "foo");
>>      qobject_decref(obj);
>> -    qobject_decref(qobj);

Dereferencing a subset of the qdict leaks the overal qdict.

> 
> Hmm...  obj is an alias for qdict, qobj is a member of qdict, freeing
> obj frees qobj (unless there's another reference to qobj I can't see).
> The line you delete then is a use-after-free bug that underflows the
> reference counter.  Correct?

Valgrind complained about a leak, not a use-after-free. But there indeed
may be more than one issue that got solved by correctly dropping the
reference at the right point in time, prior to reassigning qobj for use
as pointing to a different portion of the qdict.

> 
> If yes, commit message should mention it briefly, because this isn't
> just a leak.  Actually, I'd make it a separate commit, to keep commit
> messages simple, particularly the headlines.

I'll have to refresh my memory what else is going on, but I can indeed
split this out if it is different than a simple memleak fix.

> 
> Aside: qobject_decref() neglects to assert(!obj || obj->refcnt > 0).

Sounds like a separate patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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