[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-1.4] tests/test-string-input-visitor: Handle
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH for-1.4] tests/test-string-input-visitor: Handle errors provoked by fuzz test |
Date: |
Mon, 4 Feb 2013 14:13:16 -0200 |
On Mon, 04 Feb 2013 08:48:57 +0100
Markus Armbruster <address@hidden> wrote:
> [Note cc: Luiz]
>
> Peter Maydell <address@hidden> writes:
>
> > On 2 February 2013 21:37, Andreas Färber <address@hidden> wrote:
> >> Am 02.02.2013 22:19, schrieb Peter Maydell:
> >>> It's OK and expected for visitors to return errors when presented with
> >>> the fuzz test's random data. This means the test harness needs to
> >>> handle them; check for and free any error after each visitor call,
> >>> and only free the string returned by visit_type_str if visit_type_str
> >>> succeeded.
> >>>
> >>> This fixes a problem where this test failed the MacOSX malloc()
> >>> consistency checks and might segfault on other platforms [due
> >>> to calling free() on an uninitialized pointer variable].
> >>>
> >>> Signed-off-by: Peter Maydell <address@hidden>
> >>> ---
> >>> tests/test-string-input-visitor.c | 23 ++++++++++++++++++++++-
> >>> 1 file changed, 22 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tests/test-string-input-visitor.c
> >>> b/tests/test-string-input-visitor.c
> >>> index f6b0093..793b334 100644
> >>> --- a/tests/test-string-input-visitor.c
> >>> +++ b/tests/test-string-input-visitor.c
> >>> @@ -194,20 +194,41 @@ static void
> >>> test_visitor_in_fuzz(TestInputVisitorData *data,
> >>>
> >>> v = visitor_input_test_init(data, buf);
> >>> visit_type_int(v, &ires, NULL, &errp);
> >>> + if (error_is_set(&errp)) {
> >>> + error_free(errp);
> >>> + errp = NULL;
> >>> + }
> >>
> >> It seems to me the naming is bad here: errp appears to be an Error*, not
> >> an Error**. It would be nice to fix this within the function touched.
> >
> > "Error *errp" is blessed by docs/writing-qmp-commands.txt (and
> > git grep 'Error \*errp' has 80 examples in the tree). I think
> > if I were writing this code I'd probably agree with you about the
> > naming, but I'm not and I don't particularly feel like changing
> > names somebody else has been consistent about in this source file
> > in the course of fixing a bug.
> >
> >> Since it is an Error*, I think it was said that we should not use
> >> error_is_set() but err != NULL (or if you prefer, just err).
> >> error_is_set() is intended for **errp arguments that may be NULL.
> >
> > Calling error_is_set(&some_local_err_ptr) is also in the
> > examples in the docs. If not doing that is the recommendation
> > there should be a doc comment in error.h about that.
>
> I'd find this clearer:
>
> v = visitor_input_test_init(data, buf);
> visit_type_int(v, &ires, NULL, &errp);
> error_free(errp);
> errp = NULL;
>
> Makes it blatantly obvious that the error is freed and the pointer reset
> no matter what.
It's simpler to get the error ignored by passing errp=NULL instead:
visit_type_int(v, &ires, NULL, NULL);
For the string test, you can do:
sres = NULL;
visit_type_str(v, &sres, NULL, NULL);
g_free(sres);
Two additional comments:
o Isn't test_visitor_in_fuzz() leaking the visitors it allocates
with visitor_input_test_init()?
o This is probably the reason for the crash I reported here:
http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg05227.html