| Message ID | 1359839979-26852-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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); > } > } >
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
[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.
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
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); } }
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(-)