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

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(-)

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

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);
>      }
>  }