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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/04/2015 01:19 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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 <address@hidden>
>>>
>>> ---
>>> 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.

What we (royal we) don't have is memory going back from 06/27 to 03/27
%-}

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

I really, really like bug fixes coming with an impact assessment.
However, since this is just *tests*, I recommend *not* to spend time on
figuring out what exactly was broken, and simply adjust the commit
messages claims a bit.

>> Aside: qobject_decref() neglects to assert(!obj || obj->refcnt > 0).
>
> Sounds like a separate patch.

Definitely.