Patchwork [for-1.4] tests/test-string-input-visitor: Handle errors provoked by fuzz test

login
register
mail settings
Submitter Peter Maydell
Date Feb. 2, 2013, 9:19 p.m.
Message ID <1359839979-26852-1-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/217701/
State New
Headers show

Comments

Peter Maydell - Feb. 2, 2013, 9:19 p.m.
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 <peter.maydell@linaro.org>
---
 tests/test-string-input-visitor.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
Andreas Färber - Feb. 2, 2013, 9:37 p.m.
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 <peter.maydell@linaro.org>
> ---
>  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.

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.

Your analysis and the freeing and NULL'ing as solution looks correct.

Andreas

>  
>          v = visitor_input_test_init(data, buf);
>          visit_type_bool(v, &bres, NULL, &errp);
> +        if (error_is_set(&errp)) {
> +            error_free(errp);
> +            errp = NULL;
> +        }
>          visitor_input_teardown(data, NULL);
>  
>          v = visitor_input_test_init(data, buf);
>          visit_type_number(v, &nres, NULL, &errp);
> +        if (error_is_set(&errp)) {
> +            error_free(errp);
> +            errp = NULL;
> +        }
>  
>          v = visitor_input_test_init(data, buf);
>          visit_type_str(v, &sres, NULL, &errp);
> -        g_free(sres);
> +        if (error_is_set(&errp)) {
> +            error_free(errp);
> +            errp = NULL;
> +        } else {
> +            g_free(sres);
> +        }
>  
>          v = visitor_input_test_init(data, buf);
>          visit_type_EnumOne(v, &eres, NULL, &errp);
> +        if (error_is_set(&errp)) {
> +            error_free(errp);
> +            errp = NULL;
> +        }
>          visitor_input_teardown(data, NULL);
>      }
>  }
>
Peter Maydell - Feb. 2, 2013, 11:19 p.m.
On 2 February 2013 21:37, Andreas Färber <afaerber@suse.de> 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 <peter.maydell@linaro.org>
>> ---
>>  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.

-- PMM
Markus Armbruster - Feb. 4, 2013, 7:48 a.m.
[Note cc: Luiz]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 2 February 2013 21:37, Andreas Färber <afaerber@suse.de> 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 <peter.maydell@linaro.org>
>>> ---
>>>  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.
Luiz Capitulino - Feb. 4, 2013, 4:13 p.m.
On Mon, 04 Feb 2013 08:48:57 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> [Note cc: Luiz]
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 2 February 2013 21:37, Andreas Färber <afaerber@suse.de> 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 <peter.maydell@linaro.org>
> >>> ---
> >>>  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

Patch

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;
+        }
 
         v = visitor_input_test_init(data, buf);
         visit_type_bool(v, &bres, NULL, &errp);
+        if (error_is_set(&errp)) {
+            error_free(errp);
+            errp = NULL;
+        }
         visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
         visit_type_number(v, &nres, NULL, &errp);
+        if (error_is_set(&errp)) {
+            error_free(errp);
+            errp = NULL;
+        }
 
         v = visitor_input_test_init(data, buf);
         visit_type_str(v, &sres, NULL, &errp);
-        g_free(sres);
+        if (error_is_set(&errp)) {
+            error_free(errp);
+            errp = NULL;
+        } else {
+            g_free(sres);
+        }
 
         v = visitor_input_test_init(data, buf);
         visit_type_EnumOne(v, &eres, NULL, &errp);
+        if (error_is_set(&errp)) {
+            error_free(errp);
+            errp = NULL;
+        }
         visitor_input_teardown(data, NULL);
     }
 }