A struct's 'data' must be a JSON object defining the struct's members.
The QAPI code generator incorrectly accepts a JSON string instead, and
then crashes in QAPISchema._make_members() called from
._def_struct_type().
Fix to reject it, and add a test case.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/expr.py | 24 +++++++++++++--------
tests/qapi-schema/meson.build | 1 +
tests/qapi-schema/struct-data-typename.err | 2 ++
tests/qapi-schema/struct-data-typename.json | 2 ++
tests/qapi-schema/struct-data-typename.out | 0
5 files changed, 20 insertions(+), 9 deletions(-)
create mode 100644 tests/qapi-schema/struct-data-typename.err
create mode 100644 tests/qapi-schema/struct-data-typename.json
create mode 100644 tests/qapi-schema/struct-data-typename.out
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 9bae500a7d..cae0a08359 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -354,14 +354,14 @@ def check_type_name_or_array(value: Optional[object],
source)
-def check_type_name_or_implicit(value: Optional[object],
- info: QAPISourceInfo, source: str,
- parent_name: Optional[str]) -> None:
+def check_type_implicit(value: Optional[object],
+ info: QAPISourceInfo, source: str,
+ parent_name: Optional[str]) -> None:
"""
Normalize and validate an optional implicit struct type.
- Accept ``None``, ``str``, or a ``dict`` defining an implicit
- struct type. The latter is normalized in place.
+ Accept ``None`` or a ``dict`` defining an implicit struct type.
+ The latter is normalized in place.
:param value: The value to check.
:param info: QAPI schema source file information.
@@ -377,9 +377,6 @@ def check_type_name_or_implicit(value: Optional[object],
if value is None:
return
- if isinstance(value, str):
- return
-
if not isinstance(value, dict):
raise QAPISemError(info,
"%s should be an object or type name" % source)
@@ -401,6 +398,15 @@ def check_type_name_or_implicit(value: Optional[object],
check_type_name_or_array(arg['type'], info, key_source)
+def check_type_name_or_implicit(value: Optional[object],
+ info: QAPISourceInfo, source: str,
+ parent_name: Optional[str]) -> None:
+ if value is None or isinstance(value, str):
+ return
+
+ check_type_implicit(value, info, source, parent_name)
+
+
def check_features(features: Optional[object],
info: QAPISourceInfo) -> None:
"""
@@ -486,7 +492,7 @@ def check_struct(expr: QAPIExpression) -> None:
name = cast(str, expr['struct']) # Checked in check_exprs
members = expr['data']
- check_type_name_or_implicit(members, expr.info, "'data'", name)
+ check_type_implicit(members, expr.info, "'data'", name)
check_type_name(expr.get('base'), expr.info, "'base'")
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index d85b14f28c..f88110bddf 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -164,6 +164,7 @@ schemas = [
'struct-base-clash-deep.json',
'struct-base-clash.json',
'struct-data-invalid.json',
+ 'struct-data-typename.json',
'struct-member-if-invalid.json',
'struct-member-invalid-dict.json',
'struct-member-invalid.json',
diff --git a/tests/qapi-schema/struct-data-typename.err b/tests/qapi-schema/struct-data-typename.err
new file mode 100644
index 0000000000..8fbfe99a42
--- /dev/null
+++ b/tests/qapi-schema/struct-data-typename.err
@@ -0,0 +1,2 @@
+struct-data-typename.json: In struct 'Stru2':
+struct-data-typename.json:2: 'data' should be an object or type name
diff --git a/tests/qapi-schema/struct-data-typename.json b/tests/qapi-schema/struct-data-typename.json
new file mode 100644
index 0000000000..70fbad0ee4
--- /dev/null
+++ b/tests/qapi-schema/struct-data-typename.json
@@ -0,0 +1,2 @@
+{ 'struct': 'Stru1', 'data': {} }
+{ 'struct': 'Stru2', 'data': 'Stru1' }
diff --git a/tests/qapi-schema/struct-data-typename.out b/tests/qapi-schema/struct-data-typename.out
new file mode 100644
index 0000000000..e69de29bb2
--
2.39.2On Thu, Mar 16, 2023 at 08:13:19AM +0100, Markus Armbruster wrote:
> A struct's 'data' must be a JSON object defining the struct's members.
> The QAPI code generator incorrectly accepts a JSON string instead, and
> then crashes in QAPISchema._make_members() called from
> ._def_struct_type().
>
> Fix to reject it, and add a test case.
Nice catch; I see why the split into three functions earlier on
foreshadowed some subtle bug fixes to come.
> +++ b/scripts/qapi/expr.py
> @@ -354,14 +354,14 @@ def check_type_name_or_array(value: Optional[object],
> source)
>
>
> -def check_type_name_or_implicit(value: Optional[object],
> - info: QAPISourceInfo, source: str,
> - parent_name: Optional[str]) -> None:
> +def check_type_implicit(value: Optional[object],
> + info: QAPISourceInfo, source: str,
> + parent_name: Optional[str]) -> None:
At first I thought this was a straight rename...
> """
> Normalize and validate an optional implicit struct type.
>
> - Accept ``None``, ``str``, or a ``dict`` defining an implicit
> - struct type. The latter is normalized in place.
> + Accept ``None`` or a ``dict`` defining an implicit struct type.
> + The latter is normalized in place.
>
> :param value: The value to check.
> :param info: QAPI schema source file information.
> @@ -377,9 +377,6 @@ def check_type_name_or_implicit(value: Optional[object],
> if value is None:
> return
>
> - if isinstance(value, str):
> - return
> -
> if not isinstance(value, dict):
> raise QAPISemError(info,
> "%s should be an object or type name" % source)
> @@ -401,6 +398,15 @@ def check_type_name_or_implicit(value: Optional[object],
> check_type_name_or_array(arg['type'], info, key_source)
>
>
> +def check_type_name_or_implicit(value: Optional[object],
> + info: QAPISourceInfo, source: str,
> + parent_name: Optional[str]) -> None:
> + if value is None or isinstance(value, str):
...until I got here and saw that you kept the original name, and added
a new helper. Worth calling out the new function name
check_type_implicit() in the commit message? It would have spared me
a minute.
As earlier, you lost the doc comment. I'll leave it to your
discretion if it is important to copy one back in.
> + return
> +
> + check_type_implicit(value, info, source, parent_name)
> +
> +
> def check_features(features: Optional[object],
> info: QAPISourceInfo) -> None:
> """
> @@ -486,7 +492,7 @@ def check_struct(expr: QAPIExpression) -> None:
> name = cast(str, expr['struct']) # Checked in check_exprs
> members = expr['data']
>
> - check_type_name_or_implicit(members, expr.info, "'data'", name)
> + check_type_implicit(members, expr.info, "'data'", name)
> check_type_name(expr.get('base'), expr.info, "'base'")
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On Thu, Mar 16, 2023 at 08:13:19AM +0100, Markus Armbruster wrote:
>> A struct's 'data' must be a JSON object defining the struct's members.
>> The QAPI code generator incorrectly accepts a JSON string instead, and
>> then crashes in QAPISchema._make_members() called from
>> ._def_struct_type().
>>
>> Fix to reject it, and add a test case.
>
> Nice catch; I see why the split into three functions earlier on
> foreshadowed some subtle bug fixes to come.
>
>> +++ b/scripts/qapi/expr.py
>> @@ -354,14 +354,14 @@ def check_type_name_or_array(value: Optional[object],
>> source)
>>
>>
>> -def check_type_name_or_implicit(value: Optional[object],
>> - info: QAPISourceInfo, source: str,
>> - parent_name: Optional[str]) -> None:
>> +def check_type_implicit(value: Optional[object],
>> + info: QAPISourceInfo, source: str,
>> + parent_name: Optional[str]) -> None:
>
> At first I thought this was a straight rename...
>
>> """
>> Normalize and validate an optional implicit struct type.
>>
>> - Accept ``None``, ``str``, or a ``dict`` defining an implicit
>> - struct type. The latter is normalized in place.
>> + Accept ``None`` or a ``dict`` defining an implicit struct type.
>> + The latter is normalized in place.
>>
>> :param value: The value to check.
>> :param info: QAPI schema source file information.
>> @@ -377,9 +377,6 @@ def check_type_name_or_implicit(value: Optional[object],
>> if value is None:
>> return
>>
>> - if isinstance(value, str):
>> - return
>> -
>> if not isinstance(value, dict):
>> raise QAPISemError(info,
>> "%s should be an object or type name" % source)
>> @@ -401,6 +398,15 @@ def check_type_name_or_implicit(value: Optional[object],
>> check_type_name_or_array(arg['type'], info, key_source)
>>
>>
>> +def check_type_name_or_implicit(value: Optional[object],
>> + info: QAPISourceInfo, source: str,
>> + parent_name: Optional[str]) -> None:
>> + if value is None or isinstance(value, str):
>
> ...until I got here and saw that you kept the original name, and added
> a new helper. Worth calling out the new function name
> check_type_implicit() in the commit message? It would have spared me
> a minute.
Can do.
> As earlier, you lost the doc comment. I'll leave it to your
> discretion if it is important to copy one back in.
I didn't duplicate the doc string, which means it moves from
check_type_name_or_implicit() to check_type_implicit(), where the actual
meat is.
John, you added the doc string in commit a48653638fa (qapi/expr.py: Add
docstrings). Do you have an opinion?
>> + return
>> +
>> + check_type_implicit(value, info, source, parent_name)
>> +
>> +
>> def check_features(features: Optional[object],
>> info: QAPISourceInfo) -> None:
>> """
>> @@ -486,7 +492,7 @@ def check_struct(expr: QAPIExpression) -> None:
>> name = cast(str, expr['struct']) # Checked in check_exprs
>> members = expr['data']
>>
>> - check_type_name_or_implicit(members, expr.info, "'data'", name)
>> + check_type_implicit(members, expr.info, "'data'", name)
>> check_type_name(expr.get('base'), expr.info, "'base'")
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
© 2016 - 2025 Red Hat, Inc.