[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH for-1.4] tests/test-string-input-visitor: Handle errors provoked by fuzz test |
Date: |
Mon, 04 Feb 2013 08:48:57 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
[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.