Patchwork tests: add fuzzing to visitor tests

login
register
mail settings
Submitter Blue Swirl
Date Jan. 19, 2013, 4:01 p.m.
Message ID <25ff34ee279d98866add04ca5a7dbf4c9ea47999.1358611253.git.blauwirbel@gmail.com>
Download mbox | patch
Permalink /patch/213876/
State New
Headers show

Comments

Blue Swirl - Jan. 19, 2013, 4:01 p.m.
Perform input tests on random data.

Improvement to code coverage for qapi/string-input-visitor.c
is about 3 percentage points.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 tests/test-string-input-visitor.c |   49 +++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
Kevin Wolf - Jan. 30, 2013, 4:37 p.m.
Am 19.01.2013 17:01, schrieb Blue Swirl:
> Perform input tests on random data.
> 
> Improvement to code coverage for qapi/string-input-visitor.c
> is about 3 percentage points.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

Does this test pass for you? It consistently segfaults for me.

/string-visitor/input/fuzz: ==30703== Conditional jump or move depends
on uninitialised value(s)
==30703==    at 0x508E738: g_free (gmem.c:262)
==30703==    by 0x10B123: test_visitor_in_fuzz
(test-string-input-visitor.c:207)
==30703==    by 0x50ABCA7: g_test_run_suite_internal (gtestutils.c:1174)
==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
==30703==    by 0x50AC10E: g_test_run_suite (gtestutils.c:1282)
==30703==    by 0x108FBF: main (test-string-input-visitor.c:242)
==30703==
==30703== Conditional jump or move depends on uninitialised value(s)
==30703==    at 0x4A055B4: free (vg_replace_malloc.c:366)
==30703==    by 0x508E742: g_free (gmem.c:263)
==30703==    by 0x10B123: test_visitor_in_fuzz
(test-string-input-visitor.c:207)
==30703==    by 0x50ABCA7: g_test_run_suite_internal (gtestutils.c:1174)
==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
==30703==    by 0x50AC10E: g_test_run_suite (gtestutils.c:1282)
==30703==    by 0x108FBF: main (test-string-input-visitor.c:242)
==30703==
==30703== Invalid free() / delete / delete[]
==30703==    at 0x4A055FE: free (vg_replace_malloc.c:366)
==30703==    by 0x508E742: g_free (gmem.c:263)
==30703==    by 0x10B123: test_visitor_in_fuzz
(test-string-input-visitor.c:207)
==30703==    by 0x50ABCA7: g_test_run_suite_internal (gtestutils.c:1174)
==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
==30703==    by 0x50AC10E: g_test_run_suite (gtestutils.c:1282)
==30703==    by 0x108FBF: main (test-string-input-visitor.c:242)
==30703==  Address 0x2102508021024020 is not stack'd, malloc'd or
(recently) free'd
==30703==
Blue Swirl - Feb. 2, 2013, 12:40 p.m.
On Wed, Jan 30, 2013 at 4:37 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 19.01.2013 17:01, schrieb Blue Swirl:
>> Perform input tests on random data.
>>
>> Improvement to code coverage for qapi/string-input-visitor.c
>> is about 3 percentage points.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>
> Does this test pass for you? It consistently segfaults for me.

Yes, it works on x86_64, i386, arm and sparc64.

>
> /string-visitor/input/fuzz: ==30703== Conditional jump or move depends
> on uninitialised value(s)
> ==30703==    at 0x508E738: g_free (gmem.c:262)
> ==30703==    by 0x10B123: test_visitor_in_fuzz
> (test-string-input-visitor.c:207)
> ==30703==    by 0x50ABCA7: g_test_run_suite_internal (gtestutils.c:1174)
> ==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
> ==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
> ==30703==    by 0x50AC10E: g_test_run_suite (gtestutils.c:1282)
> ==30703==    by 0x108FBF: main (test-string-input-visitor.c:242)
> ==30703==
> ==30703== Conditional jump or move depends on uninitialised value(s)
> ==30703==    at 0x4A055B4: free (vg_replace_malloc.c:366)
> ==30703==    by 0x508E742: g_free (gmem.c:263)
> ==30703==    by 0x10B123: test_visitor_in_fuzz
> (test-string-input-visitor.c:207)
> ==30703==    by 0x50ABCA7: g_test_run_suite_internal (gtestutils.c:1174)
> ==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
> ==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
> ==30703==    by 0x50AC10E: g_test_run_suite (gtestutils.c:1282)
> ==30703==    by 0x108FBF: main (test-string-input-visitor.c:242)
> ==30703==
> ==30703== Invalid free() / delete / delete[]
> ==30703==    at 0x4A055FE: free (vg_replace_malloc.c:366)
> ==30703==    by 0x508E742: g_free (gmem.c:263)
> ==30703==    by 0x10B123: test_visitor_in_fuzz
> (test-string-input-visitor.c:207)
> ==30703==    by 0x50ABCA7: g_test_run_suite_internal (gtestutils.c:1174)
> ==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
> ==30703==    by 0x50ABE15: g_test_run_suite_internal (gtestutils.c:1233)
> ==30703==    by 0x50AC10E: g_test_run_suite (gtestutils.c:1282)
> ==30703==    by 0x108FBF: main (test-string-input-visitor.c:242)
> ==30703==  Address 0x2102508021024020 is not stack'd, malloc'd or
> (recently) free'd
> ==30703==

The call to g_free() in the fuzz function looks suspect. I used
test_visitor_in_string() as a model (which seems to have been copied
from test-qmp-input-visitor.c), is the call to g_free() correct there
either? Perhaps Paolo or Luiz would know?
Peter Maydell - Feb. 2, 2013, 9:10 p.m.
On 2 February 2013 12:40, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Wed, Jan 30, 2013 at 4:37 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 19.01.2013 17:01, schrieb Blue Swirl:
>>> Perform input tests on random data.
>>>
>>> Improvement to code coverage for qapi/string-input-visitor.c
>>> is about 3 percentage points.
>>>
>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>
>> Does this test pass for you? It consistently segfaults for me.
>
> Yes, it works on x86_64, i386, arm and sparc64.

> The call to g_free() in the fuzz function looks suspect.

On MacOSX this g_free fails the malloc system's checks:
/string-visitor/input/fuzz: test-string-input-visitor(76691) malloc:
*** error for object 0x7fff8e606b00: pointer being freed was not
allocated

This happens because you're reusing the Error* without checking
or clearing it after each call. If it's handed an Error** that
indicates an error has already occurred, visit_type_str()
does nothing, and so in test_visitor_in_fuzz() nothing has
set sres, and we try to g_free() an uninitialized pointer.

This code should either:
(a) avoid passing the visitors anything that could provoke
an error, and g_assert(!error_is_set(&errp)) after each call
(b) if errors are ok, do
  if (error_is_set(&errp)) {
      error_free(errp);
      errp = NULL;
  }
after each call.

I don't know exactly what the semantics of visit_type_str()
are [my guess is "if no error, caller must g_free() string,
otherwise no string allocated"] -- if somebody who did know
was able to write some brief docstring comments for visitor.h
that might be nice :-)

-- PMM

Patch

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 899feda..f6b0093 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -165,6 +165,53 @@  static void test_visitor_in_enum(TestInputVisitorData *data,
     data->siv = NULL;
 }
 
+/* Try to crash the visitors */
+static void test_visitor_in_fuzz(TestInputVisitorData *data,
+                                 const void *unused)
+{
+    int64_t ires;
+    bool bres;
+    double nres;
+    char *sres;
+    EnumOne eres;
+    Error *errp = NULL;
+    Visitor *v;
+    unsigned int i;
+    char buf[10000];
+
+    for (i = 0; i < 100; i++) {
+        unsigned int j;
+
+        j = g_test_rand_int_range(0, sizeof(buf) - 1);
+
+        buf[j] = '\0';
+
+        if (j != 0) {
+            for (j--; j != 0; j--) {
+                buf[j - 1] = (char)g_test_rand_int_range(0, 256);
+            }
+        }
+
+        v = visitor_input_test_init(data, buf);
+        visit_type_int(v, &ires, NULL, &errp);
+
+        v = visitor_input_test_init(data, buf);
+        visit_type_bool(v, &bres, NULL, &errp);
+        visitor_input_teardown(data, NULL);
+
+        v = visitor_input_test_init(data, buf);
+        visit_type_number(v, &nres, NULL, &errp);
+
+        v = visitor_input_test_init(data, buf);
+        visit_type_str(v, &sres, NULL, &errp);
+        g_free(sres);
+
+        v = visitor_input_test_init(data, buf);
+        visit_type_EnumOne(v, &eres, NULL, &errp);
+        visitor_input_teardown(data, NULL);
+    }
+}
+
 static void input_visitor_test_add(const char *testpath,
                                    TestInputVisitorData *data,
                                    void (*test_func)(TestInputVisitorData *data, const void *user_data))
@@ -189,6 +236,8 @@  int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_string);
     input_visitor_test_add("/string-visitor/input/enum",
                             &in_visitor_data, test_visitor_in_enum);
+    input_visitor_test_add("/string-visitor/input/fuzz",
+                            &in_visitor_data, test_visitor_in_fuzz);
 
     g_test_run();