[Qemu-devel] [PATCH 0/3] qapi: fix crash in dealloc visitor for union types

[Qemu-devel] [PATCH 0/3] qapi: fix crash in dealloc visitor for union types

[Michael Roth]

This series introduces visit_start_enum and visit_end_enum as a way
of allowing visitors to trigger generated code to bail out on visiting
union fields if the visitor implementation deems doing so to be unsafe.

See patch 1 for the circumstances that cause the segfault in the
dealloc visitor.

This is a spin-off of a patch submitted by Fam Zheng earlier. See the
thread for additional background on why we're taking this approach:

  http://thread.gmane.org/gmane.comp.emulators.qemu/296090

Fam, if you'd like to break out your iotest into another patch I
can include it as part of this series, otherwise it can be sent as
a follow-up.

 include/qapi/visitor-impl.h             |  2 ++
 include/qapi/visitor.h                  |  2 ++
 qapi/qapi-dealloc-visitor.c             |  6 ++++++
 qapi/qapi-visit-core.c                  | 15 +++++++++++++++
 scripts/qapi-visit.py                   |  6 ++++++
 tests/qapi-schema/qapi-schema-test.json | 10 ++++++++++
 tests/qapi-schema/qapi-schema-test.out  |  3 +++
 tests/test-qmp-input-strict.c           | 17 +++++++++++++++++
 8 files changed, 61 insertions(+)

[Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Michael Roth]

In some cases an input visitor might bail out on filling out a
struct for various reasons, such as missing fields when running
in strict mode. In the case of a QAPI Union type, this may lead
to cases where the .kind field which encodes the union type
is uninitialized. Subsequently, other visitors, such as the
dealloc visitor, may use this .kind value as if it were
initialized, leading to assumptions about the union type which
in this case may lead to segfaults. For example, freeing an
integer value.

However, we can generally rely on the fact that the always-present
.data void * field that we generate for these union types will
always be NULL in cases where .kind is uninitialized (at least,
there shouldn't be a reason where we'd do this purposefully).

So pass this information on to Visitor implementation via these
optional start_union/end_union interfaces so this information
can be used to guard against the situation above. We will make
use of this information in a subsequent patch for the dealloc
visitor.

Cc: qemu-stable@nongnu.org
Reported-by: Fam Zheng <famz@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 include/qapi/visitor-impl.h |  2 ++
 include/qapi/visitor.h      |  2 ++
 qapi/qapi-visit-core.c      | 15 +++++++++++++++
 scripts/qapi-visit.py       |  6 ++++++
 4 files changed, 25 insertions(+)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index ecc0183..09bb0fd 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -55,6 +55,8 @@ struct Visitor
     void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
     /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
     void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
+    bool (*start_union)(Visitor *v, bool data_present, Error **errp);
+    void (*end_union)(Visitor *v, bool data_present, Error **errp);
 };
 
 void input_type_enum(Visitor *v, int *obj, const char *strings[],
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 4a0178f..5934f59 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -58,5 +58,7 @@ void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
 void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
 void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
 void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
+bool visit_start_union(Visitor *v, bool data_present, Error **errp);
+void visit_end_union(Visitor *v, bool data_present, Error **errp);
 
 #endif
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 55f8d40..b66b93a 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -58,6 +58,21 @@ void visit_end_list(Visitor *v, Error **errp)
     v->end_list(v, errp);
 }
 
+bool visit_start_union(Visitor *v, bool data_present, Error **errp)
+{
+    if (v->start_union) {
+        return v->start_union(v, data_present, errp);
+    }
+    return true;
+}
+
+void visit_end_union(Visitor *v, bool data_present, Error **errp)
+{
+    if (v->end_union) {
+        v->end_union(v, data_present, errp);
+    }
+}
+
 void visit_optional(Visitor *v, bool *present, const char *name,
                     Error **errp)
 {
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c129697..4520da9 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -357,6 +357,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
         if (err) {
             goto out_obj;
         }
+        if (!visit_start_union(m, !!(*obj)->data, &err)) {
+            goto out_obj;
+        }
         switch ((*obj)->kind) {
 ''',
                  disc_type = disc_type,
@@ -385,6 +388,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
 out_obj:
         error_propagate(errp, err);
         err = NULL;
+        visit_end_union(m, !!(*obj)->data, &err);
+        error_propagate(errp, err);
+        err = NULL;
     }
     visit_end_struct(m, &err);
 out:
-- 
1.9.1

[Qemu-devel] [PATCH 2/3] qapi: dealloc visitor, implement visit_start_union

[Michael Roth]

If the .data field of a QAPI Union is NULL, we don't need to free
any of the union fields..

Make use of the new visit_start_union interface to access this
information and instruct the generated code to not visit these
fields when this occurs.

Cc: qemu-stable@nongnu.org
Reported-by: Fam Zheng <famz@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi/qapi-dealloc-visitor.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index dc53545..eb77078 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -162,6 +162,11 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[],
 {
 }
 
+static bool qapi_dealloc_start_union(Visitor *v, bool data_present, Error **errp)
+{
+    return data_present;
+}
+
 Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
 {
     return &v->visitor;
@@ -191,6 +196,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
     v->visitor.type_str = qapi_dealloc_type_str;
     v->visitor.type_number = qapi_dealloc_type_number;
     v->visitor.type_size = qapi_dealloc_type_size;
+    v->visitor.start_union = qapi_dealloc_start_union;
 
     QTAILQ_INIT(&v->stack);
 
-- 
1.9.1

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

[Michael Roth]

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

Cc: qemu-stable@nongnu.org
Reviewed-by: Eric Blake <eblake@redhat.com>
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 1/3] qapi: add visit_start_union and visit_end_union

[Eric Blake]

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

On 09/11/2014 05:20 PM, Michael Roth wrote:
> In some cases an input visitor might bail out on filling out a
> struct for various reasons, such as missing fields when running
> in strict mode. In the case of a QAPI Union type, this may lead
> to cases where the .kind field which encodes the union type
> is uninitialized. Subsequently, other visitors, such as the
> dealloc visitor, may use this .kind value as if it were
> initialized, leading to assumptions about the union type which
> in this case may lead to segfaults. For example, freeing an
> integer value.
> 

>  
> +bool visit_start_union(Visitor *v, bool data_present, Error **errp)
> +{
> +    if (v->start_union) {
> +        return v->start_union(v, data_present, errp);
> +    }
> +    return true;
> +}

Any rules on whether errp must be set if returning false, and must not
be set if returning true?  If so, do we need a bool return, or is errp
sufficient?


> +++ b/scripts/qapi-visit.py
> @@ -357,6 +357,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
>          if (err) {
>              goto out_obj;
>          }
> +        if (!visit_start_union(m, !!(*obj)->data, &err)) {
> +            goto out_obj;
> +        }
>          switch ((*obj)->kind) {

and if there aren't rules, then a visitor that sets err but still
returns true would result in this code not exiting early, but passing an
already-set error into the switch, which is probably not desirable.

-- 
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 2/3] qapi: dealloc visitor, implement visit_start_union

[Eric Blake]

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

On 09/11/2014 05:20 PM, Michael Roth wrote:
> If the .data field of a QAPI Union is NULL, we don't need to free
> any of the union fields..

intentional double dot?

> 
> Make use of the new visit_start_union interface to access this
> information and instruct the generated code to not visit these
> fields when this occurs.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Fam Zheng <famz@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qapi/qapi-dealloc-visitor.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index dc53545..eb77078 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -162,6 +162,11 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[],
>  {
>  }
>  
> +static bool qapi_dealloc_start_union(Visitor *v, bool data_present, Error **errp)

Long line.

> +{
> +    return data_present;

Okay, so this is a case where you can return false without setting errp.
 It would be nice to document the interactions and contract of this
visitor here or in patch 1.

-- 
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 --]

[Qemu-devel] [PATCH 4/3] qemu-iotests: Test missing "driver" key for blockdev-add

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/087     | 17 +++++++++++++++++
 tests/qemu-iotests/087.out | 13 +++++++++++++
 2 files changed, 30 insertions(+)

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 0/3] qapi: fix crash in dealloc visitor for union types

[Fam Zheng]

On Thu, 09/11 18:20, Michael Roth wrote:
> This series introduces visit_start_enum and visit_end_enum as a way
> of allowing visitors to trigger generated code to bail out on visiting
> union fields if the visitor implementation deems doing so to be unsafe.
> 
> See patch 1 for the circumstances that cause the segfault in the
> dealloc visitor.
> 
> This is a spin-off of a patch submitted by Fam Zheng earlier. See the
> thread for additional background on why we're taking this approach:
> 
>   http://thread.gmane.org/gmane.comp.emulators.qemu/296090
> 
> Fam, if you'd like to break out your iotest into another patch I
> can include it as part of this series, otherwise it can be sent as
> a follow-up.

It's a good exercise of this bug from the user interface, I'll reply my patch
to this series.

Thanks for taking care of this, Michael.

Fam

Re: [Qemu-devel] [PATCH 4/3] qemu-iotests: Test missing "driver" key for blockdev-add

[Eric Blake]

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

On 09/11/2014 09:04 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/087     | 17 +++++++++++++++++
>  tests/qemu-iotests/087.out | 13 +++++++++++++
>  2 files changed, 30 insertions(+)

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

-- 
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 1/3] qapi: add visit_start_union and visit_end_union

[Paolo Bonzini]

Il 12/09/2014 01:20, Michael Roth ha scritto:
> In some cases an input visitor might bail out on filling out a
> struct for various reasons, such as missing fields when running
> in strict mode. In the case of a QAPI Union type, this may lead
> to cases where the .kind field which encodes the union type
> is uninitialized. Subsequently, other visitors, such as the
> dealloc visitor, may use this .kind value as if it were
> initialized, leading to assumptions about the union type which
> in this case may lead to segfaults. For example, freeing an
> integer value.
> 
> However, we can generally rely on the fact that the always-present
> .data void * field that we generate for these union types will
> always be NULL in cases where .kind is uninitialized (at least,
> there shouldn't be a reason where we'd do this purposefully).
> 
> So pass this information on to Visitor implementation via these
> optional start_union/end_union interfaces so this information
> can be used to guard against the situation above. We will make
> use of this information in a subsequent patch for the dealloc
> visitor.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Fam Zheng <famz@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  include/qapi/visitor-impl.h |  2 ++
>  include/qapi/visitor.h      |  2 ++
>  qapi/qapi-visit-core.c      | 15 +++++++++++++++
>  scripts/qapi-visit.py       |  6 ++++++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index ecc0183..09bb0fd 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -55,6 +55,8 @@ struct Visitor
>      void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
>      /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
>      void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
> +    bool (*start_union)(Visitor *v, bool data_present, Error **errp);
> +    void (*end_union)(Visitor *v, bool data_present, Error **errp);
>  };
>  
>  void input_type_enum(Visitor *v, int *obj, const char *strings[],
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 4a0178f..5934f59 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -58,5 +58,7 @@ void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
>  void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
>  void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
>  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
> +bool visit_start_union(Visitor *v, bool data_present, Error **errp);
> +void visit_end_union(Visitor *v, bool data_present, Error **errp);
>  
>  #endif
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 55f8d40..b66b93a 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -58,6 +58,21 @@ void visit_end_list(Visitor *v, Error **errp)
>      v->end_list(v, errp);
>  }
>  
> +bool visit_start_union(Visitor *v, bool data_present, Error **errp)
> +{
> +    if (v->start_union) {
> +        return v->start_union(v, data_present, errp);
> +    }
> +    return true;
> +}

Should output visitors set an error, or even abort, if the data is not
present, thus avoiding a NULL-pointer dereference later on?

But I guess this is really just cosmetic, so you can add my

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

for the whole series.

Paolo

> +void visit_end_union(Visitor *v, bool data_present, Error **errp)
> +{
> +    if (v->end_union) {
> +        v->end_union(v, data_present, errp);
> +    }
> +}
> +
>  void visit_optional(Visitor *v, bool *present, const char *name,
>                      Error **errp)
>  {
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index c129697..4520da9 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -357,6 +357,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
>          if (err) {
>              goto out_obj;
>          }
> +        if (!visit_start_union(m, !!(*obj)->data, &err)) {
> +            goto out_obj;
> +        }
>          switch ((*obj)->kind) {
>  ''',
>                   disc_type = disc_type,
> @@ -385,6 +388,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
>  out_obj:
>          error_propagate(errp, err);
>          err = NULL;
> +        visit_end_union(m, !!(*obj)->data, &err);
> +        error_propagate(errp, err);
> +        err = NULL;
>      }
>      visit_end_struct(m, &err);
>  out:
> 

Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Michael Roth]

Quoting Eric Blake (2014-09-11 21:29:24)
> On 09/11/2014 05:20 PM, Michael Roth wrote:
> > In some cases an input visitor might bail out on filling out a
> > struct for various reasons, such as missing fields when running
> > in strict mode. In the case of a QAPI Union type, this may lead
> > to cases where the .kind field which encodes the union type
> > is uninitialized. Subsequently, other visitors, such as the
> > dealloc visitor, may use this .kind value as if it were
> > initialized, leading to assumptions about the union type which
> > in this case may lead to segfaults. For example, freeing an
> > integer value.
> > 
> 
> >  
> > +bool visit_start_union(Visitor *v, bool data_present, Error **errp)
> > +{
> > +    if (v->start_union) {
> > +        return v->start_union(v, data_present, errp);
> > +    }
> > +    return true;
> > +}
> 
> Any rules on whether errp must be set if returning false, and must not
> be set if returning true?  If so, do we need a bool return, or is errp
> sufficient?

No restrictions in that regard, since this just an indicator of whether
any work should be done on the union fields. For instance the dealloc
visitor should continue operating normally and just skip over these
rather than going into an error state.

> 
> 
> > +++ b/scripts/qapi-visit.py
> > @@ -357,6 +357,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
> >          if (err) {
> >              goto out_obj;
> >          }
> > +        if (!visit_start_union(m, !!(*obj)->data, &err)) {
> > +            goto out_obj;
> > +        }
> >          switch ((*obj)->kind) {
> 
> and if there aren't rules, then a visitor that sets err but still
> returns true would result in this code not exiting early, but passing an
> already-set error into the switch, which is probably not desirable.

Ahh, good catch. We should be jumping to out_obj: on error as well.

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

Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Michael Roth]

Quoting Paolo Bonzini (2014-09-12 05:17:12)
> Il 12/09/2014 01:20, Michael Roth ha scritto:
> > In some cases an input visitor might bail out on filling out a
> > struct for various reasons, such as missing fields when running
> > in strict mode. In the case of a QAPI Union type, this may lead
> > to cases where the .kind field which encodes the union type
> > is uninitialized. Subsequently, other visitors, such as the
> > dealloc visitor, may use this .kind value as if it were
> > initialized, leading to assumptions about the union type which
> > in this case may lead to segfaults. For example, freeing an
> > integer value.
> > 
> > However, we can generally rely on the fact that the always-present
> > .data void * field that we generate for these union types will
> > always be NULL in cases where .kind is uninitialized (at least,
> > there shouldn't be a reason where we'd do this purposefully).
> > 
> > So pass this information on to Visitor implementation via these
> > optional start_union/end_union interfaces so this information
> > can be used to guard against the situation above. We will make
> > use of this information in a subsequent patch for the dealloc
> > visitor.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Fam Zheng <famz@redhat.com>
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  include/qapi/visitor-impl.h |  2 ++
> >  include/qapi/visitor.h      |  2 ++
> >  qapi/qapi-visit-core.c      | 15 +++++++++++++++
> >  scripts/qapi-visit.py       |  6 ++++++
> >  4 files changed, 25 insertions(+)
> > 
> > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> > index ecc0183..09bb0fd 100644
> > --- a/include/qapi/visitor-impl.h
> > +++ b/include/qapi/visitor-impl.h
> > @@ -55,6 +55,8 @@ struct Visitor
> >      void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
> >      /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
> >      void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
> > +    bool (*start_union)(Visitor *v, bool data_present, Error **errp);
> > +    void (*end_union)(Visitor *v, bool data_present, Error **errp);
> >  };
> >  
> >  void input_type_enum(Visitor *v, int *obj, const char *strings[],
> > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> > index 4a0178f..5934f59 100644
> > --- a/include/qapi/visitor.h
> > +++ b/include/qapi/visitor.h
> > @@ -58,5 +58,7 @@ void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
> >  void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
> >  void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
> >  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
> > +bool visit_start_union(Visitor *v, bool data_present, Error **errp);
> > +void visit_end_union(Visitor *v, bool data_present, Error **errp);
> >  
> >  #endif
> > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> > index 55f8d40..b66b93a 100644
> > --- a/qapi/qapi-visit-core.c
> > +++ b/qapi/qapi-visit-core.c
> > @@ -58,6 +58,21 @@ void visit_end_list(Visitor *v, Error **errp)
> >      v->end_list(v, errp);
> >  }
> >  
> > +bool visit_start_union(Visitor *v, bool data_present, Error **errp)
> > +{
> > +    if (v->start_union) {
> > +        return v->start_union(v, data_present, errp);
> > +    }
> > +    return true;
> > +}
> 
> Should output visitors set an error, or even abort, if the data is not
> present, thus avoiding a NULL-pointer dereference later on?

I think there's at least one corner case where this might cause issues:

{ 'union': 'UserDefUnion',
  'base': 'UserDefZero',
  'data': { 'a' : 'int', 'b' : 'UserDefB' } }

If UserDefUnion.a is 0, UserDefUnion.data will cast it to a NULL value and
cause the output visitor to bail, when really it should just be left to
continue on serializing the integer.

To guard against that sort of thing I think we'd need a way to verify .kind
field is initialized, so that kind of brings us back to original problem.
But if we do decide to audit or change visitors/users to encode this
information in .kind it would be fairly simply to extend
visit_start_union/visit_end_union to pass that information along as well.

> 
> But I guess this is really just cosmetic, so you can add my
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> for the whole series.
> 
> Paolo
> 
> > +void visit_end_union(Visitor *v, bool data_present, Error **errp)
> > +{
> > +    if (v->end_union) {
> > +        v->end_union(v, data_present, errp);
> > +    }
> > +}
> > +
> >  void visit_optional(Visitor *v, bool *present, const char *name,
> >                      Error **errp)
> >  {
> > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> > index c129697..4520da9 100644
> > --- a/scripts/qapi-visit.py
> > +++ b/scripts/qapi-visit.py
> > @@ -357,6 +357,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
> >          if (err) {
> >              goto out_obj;
> >          }
> > +        if (!visit_start_union(m, !!(*obj)->data, &err)) {
> > +            goto out_obj;
> > +        }
> >          switch ((*obj)->kind) {
> >  ''',
> >                   disc_type = disc_type,
> > @@ -385,6 +388,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
> >  out_obj:
> >          error_propagate(errp, err);
> >          err = NULL;
> > +        visit_end_union(m, !!(*obj)->data, &err);
> > +        error_propagate(errp, err);
> > +        err = NULL;
> >      }
> >      visit_end_struct(m, &err);
> >  out:
> >

Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Paolo Bonzini]

Il 12/09/2014 17:34, Michael Roth ha scritto:
> 
> { 'union': 'UserDefUnion',
>   'base': 'UserDefZero',
>   'data': { 'a' : 'int', 'b' : 'UserDefB' } }
> 
> If UserDefUnion.a is 0, UserDefUnion.data will cast it to a NULL value and
> cause the output visitor to bail, when really it should just be left to
> continue on serializing the integer.

In the case of dealloc, that'd be okay because the dealloc visit would
do nothing for KIND_A, right?

Paolo

Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Michael Roth]

Quoting Paolo Bonzini (2014-09-12 10:39:49)
> Il 12/09/2014 17:34, Michael Roth ha scritto:
> > 
> > { 'union': 'UserDefUnion',
> >   'base': 'UserDefZero',
> >   'data': { 'a' : 'int', 'b' : 'UserDefB' } }
> > 
> > If UserDefUnion.a is 0, UserDefUnion.data will cast it to a NULL value and
> > cause the output visitor to bail, when really it should just be left to
> > continue on serializing the integer.
> 
> In the case of dealloc, that'd be okay because the dealloc visit would
> do nothing for KIND_A, right?

Yup that should be fine for the dealloc visitor. With this series we never
actually visit the int in this case though due to this quirk. But that's
okay because it's not an allocated type and the dealloc visitor doesn't need
to do anything anyway. (It's a bit wonky, but since that reliance on
implementation details now lives in the visitor implementation of
visit_start_union it's reasonably contained at least)

But if we're looking at extending visit_start_union for use in something like
an output visitor, this would need to be addressed some other way, since
skipping scalar fields because they're 0 is a bug there.

> 
> Paolo

Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Paolo Bonzini]

Il 12/09/2014 18:17, Michael Roth ha scritto:
> Quoting Paolo Bonzini (2014-09-12 10:39:49)
>> Il 12/09/2014 17:34, Michael Roth ha scritto:
>>>
>>> { 'union': 'UserDefUnion',
>>>   'base': 'UserDefZero',
>>>   'data': { 'a' : 'int', 'b' : 'UserDefB' } }
>>>
>>> If UserDefUnion.a is 0, UserDefUnion.data will cast it to a NULL value and
>>> cause the output visitor to bail, when really it should just be left to
>>> continue on serializing the integer.
>>
>> In the case of dealloc, that'd be okay because the dealloc visit would
>> do nothing for KIND_A, right?
> 
> Yup that should be fine for the dealloc visitor. With this series we never
> actually visit the int in this case though due to this quirk. But that's
> okay because it's not an allocated type and the dealloc visitor doesn't need
> to do anything anyway. (It's a bit wonky, but since that reliance on
> implementation details now lives in the visitor implementation of
> visit_start_union it's reasonably contained at least)
> 
> But if we're looking at extending visit_start_union for use in something like
> an output visitor, this would need to be addressed some other way, since
> skipping scalar fields because they're 0 is a bug there.

I guess it would be something like

   has_data = (kind < KIND_MAX) && (is_scalar[kind] || !!data)

That could be done in qapi-visit.py if we were so inclined...

Paolo

Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Michael Roth]

Quoting Paolo Bonzini (2014-09-12 11:29:46)
> Il 12/09/2014 18:17, Michael Roth ha scritto:
> > Quoting Paolo Bonzini (2014-09-12 10:39:49)
> >> Il 12/09/2014 17:34, Michael Roth ha scritto:
> >>>
> >>> { 'union': 'UserDefUnion',
> >>>   'base': 'UserDefZero',
> >>>   'data': { 'a' : 'int', 'b' : 'UserDefB' } }
> >>>
> >>> If UserDefUnion.a is 0, UserDefUnion.data will cast it to a NULL value and
> >>> cause the output visitor to bail, when really it should just be left to
> >>> continue on serializing the integer.
> >>
> >> In the case of dealloc, that'd be okay because the dealloc visit would
> >> do nothing for KIND_A, right?
> > 
> > Yup that should be fine for the dealloc visitor. With this series we never
> > actually visit the int in this case though due to this quirk. But that's
> > okay because it's not an allocated type and the dealloc visitor doesn't need
> > to do anything anyway. (It's a bit wonky, but since that reliance on
> > implementation details now lives in the visitor implementation of
> > visit_start_union it's reasonably contained at least)
> > 
> > But if we're looking at extending visit_start_union for use in something like
> > an output visitor, this would need to be addressed some other way, since
> > skipping scalar fields because they're 0 is a bug there.
> 
> I guess it would be something like
> 
>    has_data = (kind < KIND_MAX) && (is_scalar[kind] || !!data)
> 
> That could be done in qapi-visit.py if we were so inclined...

Yah that should be everything we'd need, but we'd need to make other changes
similar to what Fam originally proposed to ensure kind < KIND_MAX implies that
kind has actually been initialized. Or, we'd need to make all enums start at 1,
and reserve 0 for INVALID. Not aware if any option except those 2 atm.

However, we could still actually implement what you proposed for has_data as is,
and make use of the fact that even if kind happens to be invalid/uninitialized,
we still won't attempt to visit/dereference the value in an output visitor (if
they implement visit_start_union) if that value is NULL or scalar(0).

So, it makes at least one case safer. It wouldn't stop us for doing something
like serializing a char* as an integer or something along that line though, so
it's somewhat of a false assurance unless we do something to validate .kind.

> 
> Paolo