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

[Markus Armbruster]

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

Reviewed-by: Markus Armbruster <address@hidden>