Peter Xu recently made me aware of a crash bug that turned out to be
caused by disobeying string input visitor restrictions:
/*
* The string input visitor does not implement support for visiting
* QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
* of integers (except type "size") are supported.
*/
The string input visitor has few uses outside tests/. Let's review
them:
* object_property_parse()
Set a QOM property to a value parsed from a string. Uses:
- hmp_qom_set()
This is HMP command qom-set without -j. Positional arguments are
QOM path, property name and value. Can target arbitrary QOM
property, and therefore visit an arbitrary QAPI type. Crashes when
the type is one the string input visitor doesn't implement.
Reproducer: "qom-set /machine memory junk".
We created this bug when we added QOM properties with "funny" types
without fully considering the implications.
- object_apply_global_props()
Called for each -global DRIVER.PROPERTY=VALUE and for each "compat
prop".
-global can target arbitrary qdev property. Same crash risk, but I
can't see one with a visitor-crashing type offhand. Peter's "[PATCH
v3 0/4] qapi/migration: Dedup migration parameter objects and fix
tls-authz crash" adds some, e.g. "-global migration.tls-authz=junk".
"Compat props" are purely internal. Looks safe.
- object_set_propv()
Convenience interface for setting properties from C code. Usable
only with properties whose type the string input visitor can handle.
I can see maybe half a dozen uses that aren't just for testing. The
property names are all literals. Looks safe.
- pc_machine_init_sgx_epc()
Set QOM object "sgx-epc" link property "memdev" to the value of QAPI
type SgxEPC member @memdev. Visits a QAPI string, which is safe.
- x86_cpu_apply_props()
Set default CPU properties from kvm_default_props[] or
tcg_default_props[]. Looks like these are all of boolean type,
which is safe.
- x86_cpu_apply_version_props()
Set version-specific CPU properties from X86CPUDefinition member
@versions member @props. Looks like these are all of scalar type
which are safe.
* hmp_migrate_set_parameter()
This is HMP command migrate_set_parameter. Positional arguments are
the parameter name and value. QAPI types are all scalar so far, which
are safe.
Ways to fix the crash bug:
1. Lift the string input visitor restrictions
Requires new syntax for the missing QAPI types. I don't think
inventing more syntax is a good idea. We already have human-friendly
syntax for most of the missing types: keyval_parse(), used with the
QObject input visitor. Comes with restrictions (see keyval.c), but
no crashes. We commonly use it in a way that also accepts JSON,
unrestricted: qobject_input_visitor_new_str().
2. Fail instead of crash
Still have to use qom-set -j for certain properties. Fine.
Can't use -global for certain properties. Not so fine. Could create
alternative syntax that always works, like we did for qom-set.
The two ways to do human-friendly input (string input visitor
vs. keyval_parse() + QObject input visitor) both come with restrictions
that may necessitate use of JSON (fine), but when exactly that's
needed differs (not fine).
Meh.
3. Unified solution for QAPI-based human-friendly input
keyval_parse() + QObject input visitor works for complex QAPI types
only. To replace the string input visitor, we need something that
also works for the QAPI types the string input visitor can do now:
scalars and array of integers.
The restriction to complex QAPI types is in keyval_parse(): it parses
into a QDict.
I believe we can create a variant of keyval_parse() that parses into
a QObject instead.
Side note: keyval_parse()'s restriction to complex QAPI types enables
easy JSON support: qobject_input_visitor_new_str() assumes JSON when
the string starts with '{'. An unrestricted variant couldn't do
that, at least not easily. There's a reason qom-set requires a -j
flag instead of recognizing JSON automatically.
Affected stable external interfaces: as far as I can tell just
-global. QMP qom-set is not affected, since it uses the QObject
input visitor, not the string input visitor.
I think we should offer a JSON-based alternative t0 -global, both for
full generality (which our human-readable syntax cannot provide), and
because management applications are better off with JSON.
Thoughts?
