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

login
register
mail settings
Submitter Peter Maydell
Date Feb. 5, 2013, 8:44 p.m.
Message ID <1360097063-30874-1-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/218340/
State New
Headers show

Comments

Peter Maydell - Feb. 5, 2013, 8:44 p.m.
It's OK and expected for visitors to return errors when presented with
the fuzz test's random data. Since the fuzzer doesn't care about
errors, we pass in NULL rather than an Error**. This fixes a bug in
the fuzzer where it was passing the same Error** into each visitor,
with the effect that once one visitor returned an error, each later
visitor would notice that it had been passed in an Error** representing
an already set error, and do nothing.

For the case of visit_type_str() we also need to handle the case where
an error means that the visitor doesn't set our char*. We initialize
the pointer to NULL so we can safely g_free() it regardless of whether
the visitor allocated a string for us or not.

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 when
visit_type_str() failed.].

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
For 1.4 because it fixes a crash bug in the test.

v1->v2 changes: I took Luiz' suggestions for simplifying this code:
just pass NULL in as an Error** since we don't care about errors, and
NULL-init sres so g_free() works either way.

I agree with Luiz that the test leaks visitors, but since it won't leak
enough to actually cause a problem, I leave that for a post-1.4 patch,
since it's a separate bug to the one we're fixing here.

 tests/test-string-input-visitor.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Andreas Färber - Feb. 5, 2013, 8:48 p.m.
Am 05.02.2013 21:44, schrieb Peter Maydell:
> It's OK and expected for visitors to return errors when presented with
> the fuzz test's random data. Since the fuzzer doesn't care about
> errors, we pass in NULL rather than an Error**. This fixes a bug in
> the fuzzer where it was passing the same Error** into each visitor,
> with the effect that once one visitor returned an error, each later
> visitor would notice that it had been passed in an Error** representing
> an already set error, and do nothing.
> 
> For the case of visit_type_str() we also need to handle the case where
> an error means that the visitor doesn't set our char*. We initialize
> the pointer to NULL so we can safely g_free() it regardless of whether
> the visitor allocated a string for us or not.
> 
> 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 when
> visit_type_str() failed.].
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas
Luiz Capitulino - Feb. 6, 2013, 12:21 p.m.
On Tue,  5 Feb 2013 20:44:23 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> It's OK and expected for visitors to return errors when presented with
> the fuzz test's random data. Since the fuzzer doesn't care about
> errors, we pass in NULL rather than an Error**. This fixes a bug in
> the fuzzer where it was passing the same Error** into each visitor,
> with the effect that once one visitor returned an error, each later
> visitor would notice that it had been passed in an Error** representing
> an already set error, and do nothing.
> 
> For the case of visit_type_str() we also need to handle the case where
> an error means that the visitor doesn't set our char*. We initialize
> the pointer to NULL so we can safely g_free() it regardless of whether
> the visitor allocated a string for us or not.
> 
> 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 when
> visit_type_str() failed.].
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> For 1.4 because it fixes a crash bug in the test.

Applied to the qmp branch, thanks.

> 
> v1->v2 changes: I took Luiz' suggestions for simplifying this code:
> just pass NULL in as an Error** since we don't care about errors, and
> NULL-init sres so g_free() works either way.
> 
> I agree with Luiz that the test leaks visitors, but since it won't leak
> enough to actually cause a problem, I leave that for a post-1.4 patch,
> since it's a separate bug to the one we're fixing here.
> 
>  tests/test-string-input-visitor.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
> index f6b0093..5989f81 100644
> --- a/tests/test-string-input-visitor.c
> +++ b/tests/test-string-input-visitor.c
> @@ -174,7 +174,6 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
>      double nres;
>      char *sres;
>      EnumOne eres;
> -    Error *errp = NULL;
>      Visitor *v;
>      unsigned int i;
>      char buf[10000];
> @@ -193,21 +192,22 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
>          }
>  
>          v = visitor_input_test_init(data, buf);
> -        visit_type_int(v, &ires, NULL, &errp);
> +        visit_type_int(v, &ires, NULL, NULL);
>  
>          v = visitor_input_test_init(data, buf);
> -        visit_type_bool(v, &bres, NULL, &errp);
> +        visit_type_bool(v, &bres, NULL, NULL);
>          visitor_input_teardown(data, NULL);
>  
>          v = visitor_input_test_init(data, buf);
> -        visit_type_number(v, &nres, NULL, &errp);
> +        visit_type_number(v, &nres, NULL, NULL);
>  
>          v = visitor_input_test_init(data, buf);
> -        visit_type_str(v, &sres, NULL, &errp);
> +        sres = NULL;
> +        visit_type_str(v, &sres, NULL, NULL);
>          g_free(sres);
>  
>          v = visitor_input_test_init(data, buf);
> -        visit_type_EnumOne(v, &eres, NULL, &errp);
> +        visit_type_EnumOne(v, &eres, NULL, NULL);
>          visitor_input_teardown(data, NULL);
>      }
>  }

Patch

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index f6b0093..5989f81 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -174,7 +174,6 @@  static void test_visitor_in_fuzz(TestInputVisitorData *data,
     double nres;
     char *sres;
     EnumOne eres;
-    Error *errp = NULL;
     Visitor *v;
     unsigned int i;
     char buf[10000];
@@ -193,21 +192,22 @@  static void test_visitor_in_fuzz(TestInputVisitorData *data,
         }
 
         v = visitor_input_test_init(data, buf);
-        visit_type_int(v, &ires, NULL, &errp);
+        visit_type_int(v, &ires, NULL, NULL);
 
         v = visitor_input_test_init(data, buf);
-        visit_type_bool(v, &bres, NULL, &errp);
+        visit_type_bool(v, &bres, NULL, NULL);
         visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
-        visit_type_number(v, &nres, NULL, &errp);
+        visit_type_number(v, &nres, NULL, NULL);
 
         v = visitor_input_test_init(data, buf);
-        visit_type_str(v, &sres, NULL, &errp);
+        sres = NULL;
+        visit_type_str(v, &sres, NULL, NULL);
         g_free(sres);
 
         v = visitor_input_test_init(data, buf);
-        visit_type_EnumOne(v, &eres, NULL, &errp);
+        visit_type_EnumOne(v, &eres, NULL, NULL);
         visitor_input_teardown(data, NULL);
     }
 }