Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak

[Li Qiang]

Markus Armbruster <armbru@redhat.com> 于2020年7月16日周四 下午5:52写道：
>
> Li Qiang <liq3ea@gmail.com> writes:
>
> > Markus Armbruster <armbru@redhat.com> 于2020年7月16日周四 下午1:59写道：
> >>
> >> Li Qiang <liq3ea@163.com> writes:
> >>
> >> > Properly free each test response to avoid memory leak and separate
> >> > qtest_qmp() calls with spare lines, in a consistent manner.
> >> >
> >> > Fixes: 5b88849e7b9("tests/qmp-cmd-test: Add
> >> > qmp/object-add-failure-modes"
> >>
> >> The patch also fixes leaks introduced in 442b09b83d and 9fc719b869,
> >> actually.  At least it should, but the patch appears to be incomplete.
>
> 442b09b83d was fine, actually.
>
> 9fc719b869 wasn't, and your second patch hunk fixes it.  Please add a
> "Fixes: 9fc719b869' line to the commit message.

Hi Thomas,

Could you do this minor adjustment?
Add also add Markus's r-b tag.


>
> >> >
> >> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >> > Signed-off-by: Li Qiang <liq3ea@163.com>
> >> > ---
> >> > Change sincve v1: add detailed commit message
> >> >
> >> > tests/qtest/qmp-cmd-test.c | 13 +++++++++++++
> >> >  1 file changed, 13 insertions(+)
> >> >
> >> > diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
> >> > index c68f99f659..f7b1aa7fdc 100644
> >> > --- a/tests/qtest/qmp-cmd-test.c
> >> > +++ b/tests/qtest/qmp-cmd-test.c
> >> > @@ -230,6 +230,8 @@ static void test_object_add_failure_modes(void)
> >>    static void test_object_add_failure_modes(void)
> >>    {
> >>        QTestState *qts;
> >>        QDict *resp;
> >>
> >>        /* attempt to create an object without props */
> >>        qts = qtest_init(common_args);
> >>        resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> >>                         " {'qom-type': 'memory-backend-ram', 'id': 'ram1' 
> >> } }");
> >>        g_assert_nonnull(resp);
> >>        qmp_assert_error_class(resp, "GenericError");
> >>
> >> Doesn't @resp leak here, too?
> >
> > No, qmp_assert_error_class will call qobject_unref(rsp) will so will not 
> > leak.
>
> You're right.
>
> With the additional Fixes:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> > In fact, I think this is a inconsistent for 'qtest_qmp'.
> > I think we can apply this patch first and then change the
> > 'qmp_assert_error_class' or/and others
> > to free resp. And just let the caller of 'qtest_qmp' frees unref the rsp.
>
> Do you mean "not to free @resp"?

Yes, the 'qmp_assert_error_class' not free @resp.


>
> > What's your idea?
>
> Rename it to qmp_expect_error_and_unref()?

In fact I prefer qmp_assert_error_class has no reason unref @resp.
So the user of ''qtest_qmp' will has a unify processing of the @resp.
    resp = qtest_qmp()
    use resp
    free resp.

If we just rename to 'qmp_expect_error_and_unref'.
The user still should remember this. If not they will have UAF or mem
leak issue.

Thanks,
Li Qiang

>