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