[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid |
Date: |
Wed, 10 Sep 2014 20:01:04 -0500 |
User-agent: |
alot/0.3.4 |
Quoting Fam Zheng (2014-09-10 07:30:39)
> We shouldn't do anything in the switch block in enum's visit_type_
> function, when the enum data's ->kind is not valid at all. This happens
> when the dealloc visitor is called, after qmp input visitor returned
> error.
Comment is kind of confusing. Enum doesn't really have a .kind, this
is more specifically regarding QAPI Union types who are missing a valid
discriminator/.kind field.
>
> Now, the input visitor will set ->kind to <TYPE>_MAX if the value is not
> found, so that in dealloc, the switch block knows to skip calling into
> specific type's visiting functions.
Hmm, since QAPI input visitors handle the allocation for constructed types,
and generally do so with g_malloc0, I wonder if we could possibly just
make <TYPE>_INVALID correspond to 0 for discriminators? I think that way
we could avoid needed to have input_type_enum pass the state along?
In any case, since the fix doesn't break existing tests, could you break
the iotest out into a separate patch? I also don't like that the iotest
doesn't run during the normal make check, where I think we should try to
capture all our QAPI tests. I've added a test to test-qmp-input-strict.c.
Will send the patch shortly. If it looks reasonable could you add it to
your series?
>
> The added test case would trigger SIGSEGV without this fix.
>
> This crash is introduced since commit b1de5f43 (QMP: Add support for
> Archipelago).
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
> qapi/qapi-visit-core.c | 12 +++++-------
> scripts/qapi-visit.py | 6 ++++++
> tests/qemu-iotests/087 | 17 +++++++++++++++++
> tests/qemu-iotests/087.out | 13 +++++++++++++
> 4 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 55f8d40..6c46e0e 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -276,23 +276,21 @@ void input_type_enum(Visitor *v, int *obj, const char
> *strings[],
>
> visit_type_str(v, &enum_str, name, &local_err);
> if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> + enum_str = NULL;
> }
>
> while (strings[value] != NULL) {
> - if (strcmp(strings[value], enum_str) == 0) {
> + if (enum_str && strcmp(strings[value], enum_str) == 0) {
> break;
> }
> value++;
> }
>
> - if (strings[value] == NULL) {
> - error_set(errp, QERR_INVALID_PARAMETER, enum_str);
> - g_free(enum_str);
> - return;
> + if (!local_err && strings[value] == NULL) {
> + error_set(&local_err, QERR_INVALID_PARAMETER, enum_str);
> }
>
> g_free(enum_str);
> *obj = value;
> + error_propagate(errp, local_err);
> }
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index c129697..dad7561 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -379,6 +379,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> c_name=c_fun(key))
>
> ret += mcgen('''
> + case %(enum_full_value)s:
> + break;
> +''',
> + enum_full_value = generate_enum_full_value(disc_type, 'MAX'))
> +
> + ret += mcgen('''
> default:
> abort();
> }
> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> index 82c56b1..d7454d1 100755
> --- a/tests/qemu-iotests/087
> +++ b/tests/qemu-iotests/087
> @@ -218,6 +218,23 @@ run_qemu <<EOF
> { "execute": "quit" }
> EOF
>
> +echo
> +echo === Missing driver ===
> +echo
> +
> +_make_test_img -o encryption=on $size
> +run_qemu -S <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add",
> + "arguments": {
> + "options": {
> + "id": "disk"
> + }
> + }
> + }
> +{ "execute": "quit" }
> +EOF
> +
> # success, all done
> echo "*** done"
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> index 7fbee3f..f16bad0 100644
> --- a/tests/qemu-iotests/087.out
> +++ b/tests/qemu-iotests/087.out
> @@ -64,4 +64,17 @@ QMP_VERSION
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
>
> +
> +=== Missing driver ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on
> +Testing: -S
> +QMP_VERSION
> +{"return": {}}
> +{"error": {"class": "GenericError", "desc": "Invalid parameter type for
> 'driver', expected: string"}}
> +{"return": {}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "SHUTDOWN"}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
> +
> *** done
> --
> 1.9.3
- [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Fam Zheng, 2014/09/10
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Paolo Bonzini, 2014/09/10
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Fam Zheng, 2014/09/10
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Paolo Bonzini, 2014/09/10
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Fam Zheng, 2014/09/10
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Eric Blake, 2014/09/11
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Fam Zheng, 2014/09/11
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Michael Roth, 2014/09/11
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Paolo Bonzini, 2014/09/11
- Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid, Michael Roth, 2014/09/11
Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid,
Michael Roth <=
[Qemu-devel] [PATCH] tests: add QMP input visitor test for unions with no discriminator, Michael Roth, 2014/09/10