Eric Blake <ebl...@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 <kcham...@redhat.com>
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Markus Armbruster <arm...@redhat.com>
