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

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

[Fam Zheng]

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.

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.

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

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

[Paolo Bonzini]

Il 10/09/2014 14:30, Fam Zheng ha scritto:
> 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.
> 
> 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.
> 
> 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
> 

A bit hackish, but I don't have any better idea.

Hmm... what about adding a new member to the visitors for "invalid enum"
value?  The dealloc visitor could override it to do nothing, while the
default could abort or set an error.  Would that work?

Paolo

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

[Fam Zheng]

On Wed, 09/10 15:01, Paolo Bonzini wrote:
> Il 10/09/2014 14:30, Fam Zheng ha scritto:
> > 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.
> > 
> > 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.
> > 
> > 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
> > 
> 
> A bit hackish, but I don't have any better idea.
> 
> Hmm... what about adding a new member to the visitors for "invalid enum"
> value?  The dealloc visitor could override it to do nothing, while the
> default could abort or set an error.  Would that work?
> 

The invalid state of enum still needs to be saved in the data.  It is detected
by the input visitor, but should be checked by other visitors (output, dealloc)
later.

Fam

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

[Paolo Bonzini]

Il 10/09/2014 17:02, Fam Zheng ha scritto:
> > A bit hackish, but I don't have any better idea.
> > 
> > Hmm... what about adding a new member to the visitors for "invalid enum"
> > value?  The dealloc visitor could override it to do nothing, while the
> > default could abort or set an error.  Would that work?
> 
> The invalid state of enum still needs to be saved in the data.  It is detected
> by the input visitor, but should be checked by other visitors (output, dealloc)
> later.

Yes, that's fine.  The only part where I'm not sure is the special
casing of the _MAX enum.

Paolo

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

[Fam Zheng]

On Wed, 09/10 17:32, Paolo Bonzini wrote:
> Il 10/09/2014 17:02, Fam Zheng ha scritto:
> > > A bit hackish, but I don't have any better idea.
> > > 
> > > Hmm... what about adding a new member to the visitors for "invalid enum"
> > > value?  The dealloc visitor could override it to do nothing, while the
> > > default could abort or set an error.  Would that work?
> > 
> > The invalid state of enum still needs to be saved in the data.  It is detected
> > by the input visitor, but should be checked by other visitors (output, dealloc)
> > later.
> 
> Yes, that's fine.  The only part where I'm not sure is the special
> casing of the _MAX enum.
> 

Yes, it is abusing. Let's add an _INVALID = 0 enum which is much clearer.

Fam

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

[Michael Roth]

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

[Qemu-devel] [PATCH] tests: add QMP input visitor test for unions with no discriminator

[Michael Roth]

This more an exercise of the dealloc visitor, where it may erroneously
use an uninitialized discriminator field as indication that union
fields corresponding to that disciminator field/type are present,
which can lead to attempts to free random chunks of heap memory.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 tests/qapi-schema/qapi-schema-test.json | 10 ++++++++++
 tests/qapi-schema/qapi-schema-test.out  |  3 +++
 tests/test-qmp-input-strict.c           | 17 +++++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index ab4d3d9..d43b5fd 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -33,6 +33,9 @@
 { 'type': 'UserDefB',
   'data': { 'integer': 'int' } }
 
+{ 'type': 'UserDefC',
+  'data': { 'string1': 'str', 'string2': 'str' } }
+
 { 'union': 'UserDefUnion',
   'base': 'UserDefZero',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
@@ -47,6 +50,13 @@
 # FIXME generated struct UserDefFlatUnion has members for direct base
 # UserDefOne, but lacks members for indirect base UserDefZero
 
+# this variant of UserDefFlatUnion defaults to a union that uses fields with
+# allocated types to test corner cases in the cleanup/dealloc visitor
+{ 'union': 'UserDefFlatUnion2',
+  'base': 'UserDefUnionBase',
+  'discriminator': 'enum1',
+  'data': { 'value1' : 'UserDefC', 'value2' : 'UserDefB', 'value3' : 'UserDefA' } }
+
 { 'union': 'UserDefAnonUnion',
   'discriminator': {},
   'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 95e9899..08d7304 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -6,9 +6,11 @@
  OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
  OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
+ OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'), ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]),
  OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
@@ -32,6 +34,7 @@
  OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
  OrderedDict([('type', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))])]
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 0f77003..d5360c6 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -260,6 +260,21 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data,
     qapi_free_UserDefFlatUnion(tmp);
 }
 
+static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
+                                                     const void *unused)
+{
+    UserDefFlatUnion2 *tmp = NULL;
+    Error *err = NULL;
+    Visitor *v;
+
+    /* test situation where discriminator field ('enum1' here) is missing */
+    v = validate_test_init(data, "{ 'string': 'c', 'string1': 'd', 'string2': 'e' }");
+
+    visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err);
+    g_assert(err);
+    qapi_free_UserDefFlatUnion2(tmp);
+}
+
 static void test_validate_fail_union_anon(TestInputVisitorData *data,
                                           const void *unused)
 {
@@ -310,6 +325,8 @@ int main(int argc, char **argv)
                        &testdata, test_validate_fail_union);
     validate_test_add("/visitor/input-strict/fail/union-flat",
                        &testdata, test_validate_fail_union_flat);
+    validate_test_add("/visitor/input-strict/fail/union-flat-no-discriminator",
+                       &testdata, test_validate_fail_union_flat_no_discrim);
     validate_test_add("/visitor/input-strict/fail/union-anon",
                        &testdata, test_validate_fail_union_anon);
 
-- 
1.9.1

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On 09/10/2014 06:53 PM, Fam Zheng wrote:
> On Wed, 09/10 17:32, Paolo Bonzini wrote:
>> Il 10/09/2014 17:02, Fam Zheng ha scritto:
>>>> A bit hackish, but I don't have any better idea.
>>>>
>>>> Hmm... what about adding a new member to the visitors for "invalid enum"
>>>> value?  The dealloc visitor could override it to do nothing, while the
>>>> default could abort or set an error.  Would that work?
>>>
>>> The invalid state of enum still needs to be saved in the data.  It is detected
>>> by the input visitor, but should be checked by other visitors (output, dealloc)
>>> later.
>>
>> Yes, that's fine.  The only part where I'm not sure is the special
>> casing of the _MAX enum.
>>
> 
> Yes, it is abusing. Let's add an _INVALID = 0 enum which is much clearer.

If I understand correctly, you mean that for:

{ 'enum': 'Foo', 'data': [ 'one', 'two' ] }

FOO_ONE would now be 1 instead of its current value of 0?

We just barely saw a case where Hu Tao's code was relying on the
implicit value 0 assigned to the first enum in the json file [1]
although I strongly argued that it should be nuked (and so it was fixed
in [2]).  So I could live with reserving 0 for internal use for flagging
parse errors (such as attempting to pass the string 'three' where a Foo
value is expected).

[1] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01691.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01938.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH] tests: add QMP input visitor test for unions with no discriminator

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On 09/10/2014 07:02 PM, Michael Roth wrote:
> This more an exercise of the dealloc visitor, where it may erroneously

This is more of an exercise...

> use an uninitialized discriminator field as indication that union
> fields corresponding to that disciminator field/type are present,

discriminator

> which can lead to attempts to free random chunks of heap memory.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 10 ++++++++++
>  tests/qapi-schema/qapi-schema-test.out  |  3 +++
>  tests/test-qmp-input-strict.c           | 17 +++++++++++++++++
>  3 files changed, 30 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

[conflicts with my stalled attempt to nuke inline sub-structs in json
files, where I've promised to send a v4 - guess I'll be rebasing as part
of reviving that series...]

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Fam Zheng]

On Wed, 09/10 22:17, Eric Blake wrote:
> On 09/10/2014 06:53 PM, Fam Zheng wrote:
> > On Wed, 09/10 17:32, Paolo Bonzini wrote:
> >> Il 10/09/2014 17:02, Fam Zheng ha scritto:
> >>>> A bit hackish, but I don't have any better idea.
> >>>>
> >>>> Hmm... what about adding a new member to the visitors for "invalid enum"
> >>>> value?  The dealloc visitor could override it to do nothing, while the
> >>>> default could abort or set an error.  Would that work?
> >>>
> >>> The invalid state of enum still needs to be saved in the data.  It is detected
> >>> by the input visitor, but should be checked by other visitors (output, dealloc)
> >>> later.
> >>
> >> Yes, that's fine.  The only part where I'm not sure is the special
> >> casing of the _MAX enum.
> >>
> > 
> > Yes, it is abusing. Let's add an _INVALID = 0 enum which is much clearer.
> 
> If I understand correctly, you mean that for:
> 
> { 'enum': 'Foo', 'data': [ 'one', 'two' ] }
> 
> FOO_ONE would now be 1 instead of its current value of 0?
> 
> We just barely saw a case where Hu Tao's code was relying on the
> implicit value 0 assigned to the first enum in the json file [1]
> although I strongly argued that it should be nuked (and so it was fixed
> in [2]).  So I could live with reserving 0 for internal use for flagging
> parse errors (such as attempting to pass the string 'three' where a Foo
> value is expected).
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01691.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01938.html
> 

A closer looking shows me a huge risk with _lookup table. We have for loops
starting with index 0 all over the place to peek info _lookup arrays, but in
this case it is _INVALID and shouldn't be looked at.

Maybe we have to put _INVALID at the end after _MAX.

Fam

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

[Michael Roth]

Quoting Fam Zheng (2014-09-10 23:38:03)
> On Wed, 09/10 22:17, Eric Blake wrote:
> > On 09/10/2014 06:53 PM, Fam Zheng wrote:
> > > On Wed, 09/10 17:32, Paolo Bonzini wrote:
> > >> Il 10/09/2014 17:02, Fam Zheng ha scritto:
> > >>>> A bit hackish, but I don't have any better idea.
> > >>>>
> > >>>> Hmm... what about adding a new member to the visitors for "invalid enum"
> > >>>> value?  The dealloc visitor could override it to do nothing, while the
> > >>>> default could abort or set an error.  Would that work?
> > >>>
> > >>> The invalid state of enum still needs to be saved in the data.  It is detected
> > >>> by the input visitor, but should be checked by other visitors (output, dealloc)
> > >>> later.
> > >>
> > >> Yes, that's fine.  The only part where I'm not sure is the special
> > >> casing of the _MAX enum.
> > >>
> > > 
> > > Yes, it is abusing. Let's add an _INVALID = 0 enum which is much clearer.
> > 
> > If I understand correctly, you mean that for:
> > 
> > { 'enum': 'Foo', 'data': [ 'one', 'two' ] }
> > 
> > FOO_ONE would now be 1 instead of its current value of 0?
> > 
> > We just barely saw a case where Hu Tao's code was relying on the
> > implicit value 0 assigned to the first enum in the json file [1]
> > although I strongly argued that it should be nuked (and so it was fixed
> > in [2]).  So I could live with reserving 0 for internal use for flagging
> > parse errors (such as attempting to pass the string 'three' where a Foo
> > value is expected).
> > 
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01691.html
> > [2] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01938.html
> > 
> 
> A closer looking shows me a huge risk with _lookup table. We have for loops
> starting with index 0 all over the place to peek info _lookup arrays, but in
> this case it is _INVALID and shouldn't be looked at.

If we fix up the generator code to fill the 0 entry with a "_qapi_invalid"
string or something of the like then we shouldn't trigger any issues iterating
the lookup arrays.

Also, the .kind field of a QAPI Union type is something we generate for use
by the generated visitor code. In the case of an unspecified discriminator
we generated the enum type for that field internally. In the case where it's
specified, we use an existing enum instead...

But nothing stops us from generating a new "shadow" enum in this case as well,
with the indexes/integer values of the corresponding strings shifted by one so
we can reserve the 0 index for _INVALID. I think we can reasonably expect that
nothing outside the generated code makes use of those integer values in this
special case, and don't have to change all enum types to make that work.

> 
> Maybe we have to put _INVALID at the end after _MAX.
> 
> Fam

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

[Paolo Bonzini]

Il 11/09/2014 16:26, Michael Roth ha scritto:
> Also, the .kind field of a QAPI Union type is something we generate for use
> by the generated visitor code. In the case of an unspecified discriminator
> we generated the enum type for that field internally. In the case where it's
> specified, we use an existing enum instead...
> 
> But nothing stops us from generating a new "shadow" enum in this case as well,
> with the indexes/integer values of the corresponding strings shifted by one so
> we can reserve the 0 index for _INVALID. I think we can reasonably expect that
> nothing outside the generated code makes use of those integer values in this
> special case, and don't have to change all enum types to make that work.

But how would users fill in structs if you have to use a different enum?

What about making adding visit_start_union/visit_end_union?
visit_start_union can return false if the visit of the union has to be
skipped.

The dealloc visitor can skip it if the data field is NULL; everything
else can just use a default implementation which always returns true.

Paolo

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

[Michael Roth]

Quoting Paolo Bonzini (2014-09-11 09:35:58)
> Il 11/09/2014 16:26, Michael Roth ha scritto:
> > Also, the .kind field of a QAPI Union type is something we generate for use
> > by the generated visitor code. In the case of an unspecified discriminator
> > we generated the enum type for that field internally. In the case where it's
> > specified, we use an existing enum instead...
> > 
> > But nothing stops us from generating a new "shadow" enum in this case as well,
> > with the indexes/integer values of the corresponding strings shifted by one so
> > we can reserve the 0 index for _INVALID. I think we can reasonably expect that
> > nothing outside the generated code makes use of those integer values in this
> > special case, and don't have to change all enum types to make that work.
> 
> But how would users fill in structs if you have to use a different enum?

Argh, of course, we do still make direct use of these going in the other
direction. Those users would need to use the "shadow" enum values to make
it work, which is probably way too messy.

> 
> What about making adding visit_start_union/visit_end_union?
> visit_start_union can return false if the visit of the union has to be
> skipped.
> 
> The dealloc visitor can skip it if the data field is NULL; everything
> else can just use a default implementation which always returns true.

I forgot we had a void *data there as well. So we're basically relying
on .data != NULL implying that .kind has been properly initialized,
rather than needing to encode anything into .kind... nice.

I can imagine a case where we allocate memory for a set of union fields
(so .data != NULL) and then leave .kind uninitialized, which can still
lead to segfaults due to improper casts in the dealloc visitor, but I
don't really see a way around that. Even if we reserve .kind == 0 for
this purpose, it's still up to the user or visitor implementation to
0-initialize everything (though that's a bit easier to enforce).

So this seems like a good approach. I've ahead and hacked something up
which I'll send out shortly.

> 
> Paolo