[Qemu-devel] [PATCH] qapi: Fix crash on missing alternate member of QAPI struct

[Qemu-devel] [PATCH] qapi: Fix crash on missing alternate member of QAPI struct

[Eric Blake]

If a QAPI struct has a mandatory alternate member which is not
present on input, the input visitor reports an error for the
missing alternate without setting the discriminator, but the
cleanup code for the struct still tries to use the dealloc
visitor to clean up the alternate.

Commit dbf11922 changed visit_start_alternate to set *obj to NULL
when an error occurs, where it was previously left untouched.
Thus, before the patch, the dealloc visitor is blindly trying to
cleanup whatever branch corresponds to (*obj)->type == 0 (that is,
QTYPE_NONE, because *obj still pointed to zeroed memory), which
selects the default branch of the switch and sets an error, but
this second error is ignored by the way the dealloc visitor is
used; but after the patch, the attempt to switch dereferences NULL.

When cleaning up after a partial object parse, we specifically
check for !*obj after visit_start_struct() (see gen_visit_object());
doing the same for alternates fixes the crash. Enhance the testsuite
to give coverage for both missing struct and missing alternate
members. Also add an abort - we expect visit_start_alternate() to
either set an error or to set (*obj)->type to a valid QType that
corresponds to actual user input, and QTYPE_NONE should never
be reachable from valid input.  Had the abort() been in place
earlier, we might have noticed the dealloc visitor dereferencing
bogus zeroed memory prior to when commit dbf11922 forced our hand
by setting *obj to NULL and causing a fault.

Test case:

{'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}}

The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat
struct, which has a mandatory 'file':'BlockdevRef' in QAPI.  Since
'file' is missing as a sibling of 'driver', this should report a
graceful error rather than fault.  After this patch, we are back to:

{"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}}

Generated code in qapi-visit.c changes as:

|@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v,
|     if (err) {
|         goto out;
|     }
|+    if (!*obj) {
|+        goto out_obj;
|+    }
|     switch ((*obj)->type) {
|     case QTYPE_QDICT:
|         visit_start_struct(v, name, NULL, 0, &err);
|@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v,
|     case QTYPE_QSTRING:
|         visit_type_str(v, name, &(*obj)->u.reference, &err);
|         break;
|+    case QTYPE_NONE:
|+        abort();
|     default:
|         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
|                    "BlockdevRef");
|     }
|+out_obj:
|     visit_end_alternate(v);

Reported by Kashyap Chamarthy <kchamart@redhat.com>
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-visit.py          |  6 ++++++
 tests/test-qmp-input-visitor.c | 12 ++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 70ea8ca..ffb635c 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,6 +172,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     if (err) {
         goto out;
     }
+    if (!*obj) {
+        goto out_obj;
+    }
     switch ((*obj)->type) {
 ''',
                  c_name=c_name(name), promote_int=promote_int)
@@ -206,10 +209,13 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
 ''')

     ret += mcgen('''
+    case QTYPE_NONE:
+        abort();
     default:
         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "%(name)s");
     }
+out_obj:
     visit_end_alternate(v);
     if (err && visit_is_input(v)) {
         qapi_free_%(c_name)s(*obj);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 3b6b39e..1a4585c 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -766,6 +766,8 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
     Error *err = NULL;
     Visitor *v;
     strList *q = NULL;
+    UserDefTwo *r = NULL;
+    WrapAlternate *s = NULL;

     v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', "
                                 "'string': -42 }");
@@ -778,6 +780,16 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
     visit_type_strList(v, NULL, &q, &err);
     error_free_or_abort(&err);
     assert(!q);
+
+    v = visitor_input_test_init(data, "{ 'str':'hi' }");
+    visit_type_UserDefTwo(v, NULL, &r, &err);
+    error_free_or_abort(&err);
+    assert(!r);
+
+    v = visitor_input_test_init(data, "{ }");
+    visit_type_WrapAlternate(v, NULL, &s, &err);
+    error_free_or_abort(&err);
+    assert(!s);
 }

 static void test_visitor_in_wrong_type(TestInputVisitorData *data,
-- 
2.5.5

Re: [Qemu-devel] [PATCH] qapi: Fix crash on missing alternate member of QAPI struct

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> If a QAPI struct has a mandatory alternate member which is not
> present on input, the input visitor reports an error for the
> missing alternate without setting the discriminator, but the
> cleanup code for the struct still tries to use the dealloc
> visitor to clean up the alternate.
>
> Commit dbf11922 changed visit_start_alternate to set *obj to NULL
> when an error occurs, where it was previously left untouched.
> Thus, before the patch, the dealloc visitor is blindly trying to
> cleanup whatever branch corresponds to (*obj)->type == 0 (that is,
> QTYPE_NONE, because *obj still pointed to zeroed memory), which
> selects the default branch of the switch and sets an error, but
> this second error is ignored by the way the dealloc visitor is
> used; but after the patch, the attempt to switch dereferences NULL.
>
> When cleaning up after a partial object parse, we specifically
> check for !*obj after visit_start_struct() (see gen_visit_object());
> doing the same for alternates fixes the crash. Enhance the testsuite
> to give coverage for both missing struct and missing alternate
> members.

Paragraph break?

>          Also add an abort - we expect visit_start_alternate() to
> either set an error or to set (*obj)->type to a valid QType that
> corresponds to actual user input, and QTYPE_NONE should never
> be reachable from valid input.

Well, it shouldn't be reachable for *any* input.  But we get to the
abort() only for input deemed valid by visit_start_alternate().

>                                 Had the abort() been in place
> earlier, we might have noticed the dealloc visitor dereferencing
> bogus zeroed memory prior to when commit dbf11922 forced our hand
> by setting *obj to NULL and causing a fault.
>
> Test case:
>
> {'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}}
>
> The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat
> struct, which has a mandatory 'file':'BlockdevRef' in QAPI.  Since
> 'file' is missing as a sibling of 'driver', this should report a
> graceful error rather than fault.  After this patch, we are back to:
>
> {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}}
>
> Generated code in qapi-visit.c changes as:
>
> |@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v,
> |     if (err) {
> |         goto out;
> |     }
> |+    if (!*obj) {
> |+        goto out_obj;
> |+    }

!err && !*obj can happen with a dealloc visit.  This is something I
re-learn every other time I look at this code %-}

> |     switch ((*obj)->type) {
> |     case QTYPE_QDICT:
> |         visit_start_struct(v, name, NULL, 0, &err);
> |@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v,
> |     case QTYPE_QSTRING:
> |         visit_type_str(v, name, &(*obj)->u.reference, &err);
> |         break;
> |+    case QTYPE_NONE:
> |+        abort();
> |     default:
> |         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> |                    "BlockdevRef");
> |     }
> |+out_obj:
> |     visit_end_alternate(v);
>
> Reported by Kashyap Chamarthy <kchamart@redhat.com>
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] qapi: Fix crash on missing alternate member of QAPI struct

[Kashyap Chamarthy]

On Wed, Jun 15, 2016 at 11:37:51AM -0600, Eric Blake wrote:
> If a QAPI struct has a mandatory alternate member which is not
> present on input, the input visitor reports an error for the
> missing alternate without setting the discriminator, but the
> cleanup code for the struct still tries to use the dealloc
> visitor to clean up the alternate.
> 
> Commit dbf11922 changed visit_start_alternate to set *obj to NULL
> when an error occurs, where it was previously left untouched.
> Thus, before the patch, the dealloc visitor is blindly trying to
> cleanup whatever branch corresponds to (*obj)->type == 0 (that is,
> QTYPE_NONE, because *obj still pointed to zeroed memory), which
> selects the default branch of the switch and sets an error, but
> this second error is ignored by the way the dealloc visitor is
> used; but after the patch, the attempt to switch dereferences NULL.
> 
> When cleaning up after a partial object parse, we specifically
> check for !*obj after visit_start_struct() (see gen_visit_object());
> doing the same for alternates fixes the crash. Enhance the testsuite
> to give coverage for both missing struct and missing alternate
> members. Also add an abort - we expect visit_start_alternate() to
> either set an error or to set (*obj)->type to a valid QType that
> corresponds to actual user input, and QTYPE_NONE should never
> be reachable from valid input.  Had the abort() been in place
> earlier, we might have noticed the dealloc visitor dereferencing
> bogus zeroed memory prior to when commit dbf11922 forced our hand
> by setting *obj to NULL and causing a fault.
> 
> Test case:
> 
> {'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}}
> 
> The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat
> struct, which has a mandatory 'file':'BlockdevRef' in QAPI.  Since
> 'file' is missing as a sibling of 'driver', this should report a
> graceful error rather than fault.  After this patch, we are back to:
> 
> {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}}
> 
> Generated code in qapi-visit.c changes as:
> 
> |@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v,
> |     if (err) {
> |         goto out;
> |     }
> |+    if (!*obj) {
> |+        goto out_obj;
> |+    }
> |     switch ((*obj)->type) {
> |     case QTYPE_QDICT:
> |         visit_start_struct(v, name, NULL, 0, &err);
> |@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v,
> |     case QTYPE_QSTRING:
> |         visit_type_str(v, name, &(*obj)->u.reference, &err);
> |         break;
> |+    case QTYPE_NONE:
> |+        abort();
> |     default:
> |         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> |                    "BlockdevRef");
> |     }
> |+out_obj:
> |     visit_end_alternate(v);
> 
> Reported by Kashyap Chamarthy <kchamart@redhat.com>
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/qapi-visit.py          |  6 ++++++
>  tests/test-qmp-input-visitor.c | 12 ++++++++++++
>  2 files changed, 18 insertions(+)

Thanks for the detailed analysis, and fix.  I tested with this patch on
current Git master.  With this fix, when there's a missing 'file'
argument, a graceful error is thrown.

Along with the test you mentioned in the commit message, I tried these
two:

    QMP> {"execute": "blockdev-add",
          "arguments": {"options" : {"driver": "raw",
                                     "id": "drive-ide1-0-0",
                                     "file": {"driver": "raw",
                                              "filename": "/tmp/test1.raw"}}}}

    QMP> {"execute": "blockdev-add",
              "arguments": {"options" : {"driver": "qcow2",
                                         "id": "drive-ide2-0-0",
                                         "file": {"driver": "qcow2",
                                                  "filename": "/tmp/test2.qcow2"}}}}

All of the above now throw (rightfully):

    {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}}

Tested with:

    v2.6.0-1025-g38f1ffb
 
Tested-by: Kashyap Chamarthy <kchamart@redhat.com>

-- 
/kashyap