Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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 <famz@redhat.com>
> ---
>  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