[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-stable] [PATCH 35/56] qapi: Fix crash on missing alternate member
From: |
Michael Roth |
Subject: |
[Qemu-stable] [PATCH 35/56] qapi: Fix crash on missing alternate member of QAPI struct |
Date: |
Mon, 8 Aug 2016 16:04:06 -0500 |
From: Eric Blake <address@hidden>
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>
Message-Id: <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Tested-by: Kashyap Chamarthy <address@hidden>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <address@hidden>
(cherry picked from commit 9b4e38fe6a35890bb1d995316d7be08de0b30ee5)
Conflicts:
tests/test-qmp-input-visitor.c
* removed contexual/functional dependencies on 68ab47e
Signed-off-by: Michael Roth <address@hidden>
---
scripts/qapi-visit.py | 6 ++++++
tests/test-qmp-input-visitor.c | 14 ++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 31d2330..6c1c1fb 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -170,6 +170,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)
@@ -203,10 +206,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);
out:
error_propagate(errp, err);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 80527eb..8523283 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -739,6 +739,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 }");
@@ -757,6 +759,18 @@ static void test_visitor_in_errors(TestInputVisitorData
*data,
error_free_or_abort(&err);
assert(q);
qapi_free_strList(q);
+
+ v = visitor_input_test_init(data, "{ 'str':'hi' }");
+ visit_type_UserDefTwo(v, NULL, &r, &err);
+ error_free_or_abort(&err);
+ assert(r);
+ qapi_free_UserDefTwo(r);
+
+ v = visitor_input_test_init(data, "{ }");
+ visit_type_WrapAlternate(v, NULL, &s, &err);
+ error_free_or_abort(&err);
+ assert(s);
+ qapi_free_WrapAlternate(s);
}
static void test_visitor_in_wrong_type(TestInputVisitorData *data,
--
1.9.1
- [Qemu-stable] [PATCH 30/56] io: remove mistaken call to object_ref on QTask, (continued)
- [Qemu-stable] [PATCH 30/56] io: remove mistaken call to object_ref on QTask, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 33/56] backup: Don't leak BackupBlockJob in error path, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 31/56] ui: fix regression in printing VNC host/port on startup, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 29/56] vmsvga: don't process more than 1024 fifo commands at once, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 25/56] block: Drop bdrv_ioctl_bh_cb, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 27/56] vmsvga: add more fifo checks, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 28/56] vmsvga: shadow fifo registers, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 02/56] spice/gl: add & use qemu_spice_gl_monitor_config, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 34/56] qcow2: Avoid making the L1 table too big, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 32/56] net: fix qemu_announce_self not emitting packets, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 35/56] qapi: Fix crash on missing alternate member of QAPI struct,
Michael Roth <=
- [Qemu-stable] [PATCH 36/56] pci-assign: Move "Invalid ROM" error message to pci-assign-load-rom.c, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 37/56] vfio/pci: Fix VGA quirks, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 38/56] nbd: Allow larger requests, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 39/56] scsi-generic: Merge block max xfer len in INQUIRY response, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 40/56] scsi: Advertise limits by blocksize, not 512, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 41/56] target-sparc: fix register corruption in ldstub if there is no write permission, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 44/56] s390x/ipl: fix reboots for migration from different bios, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 46/56] qemu-iotests: Test naming of throttling groups, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 03/56] vl: change runstate only if new state is different from current state, Michael Roth, 2016/08/08
- [Qemu-stable] [PATCH 45/56] blockdev: Fix regression with the default naming of throttling groups, Michael Roth, 2016/08/08