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 <address@hidden>
> CC: address@hidden
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  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 <address@hidden>

-- 
/kashyap