[Qemu-devel] QMP accepts double dict keys

[Qemu-devel] QMP accepts double dict keys

[Max Reitz]

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

Hi,

QMP accepts double keys in dicts without complaining.  The value it is
using is apparently the last one specified:

$ qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 3},
"package": "v3.1.0-rc3-7-g87a45d86ed"}, "capabilities": []}}
{'execute':'qmp_capabilities'}
{"return": {}}
{'execute':'blockdev-add','arguments':{'driver':'null-co',
 'node-name':'foo','node-name':'bar'}}
{"return": {}}
{'execute':'query-named-block-nodes'}
{"return": [{ [...] "node-name": "bar" [...] }]}

Another test case is iotest 229 which specifies both mode=absolute-paths
and mode=existing (it wants the latter).

Max


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

Re: [Qemu-devel] QMP accepts double dict keys

[Eric Blake]

On 12/3/18 10:30 AM, Max Reitz wrote:
> Hi,
> 
> QMP accepts double keys in dicts without complaining.  The value it is
> using is apparently the last one specified:

JSON says it is undefined what happens when a client passes double keys. 
  We are probably best off if we teach our parser to be strict and 
reject doubled keys in QMP as invalid.

Hmm - can a client abuse QMP with duplicate keys to cause qemu to leak 
memory?

> 
> $ qemu-system-x86_64 -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 3},
> "package": "v3.1.0-rc3-7-g87a45d86ed"}, "capabilities": []}}
> {'execute':'qmp_capabilities'}
> {"return": {}}
> {'execute':'blockdev-add','arguments':{'driver':'null-co',
>   'node-name':'foo','node-name':'bar'}}
> {"return": {}}
> {'execute':'query-named-block-nodes'}
> {"return": [{ [...] "node-name": "bar" [...] }]}
> 
> Another test case is iotest 229 which specifies both mode=absolute-paths
> and mode=existing (it wants the latter).
> 
> Max
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] QMP accepts double dict keys

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/3/18 10:30 AM, Max Reitz wrote:
>> Hi,
>>
>> QMP accepts double keys in dicts without complaining.  The value it is
>> using is apparently the last one specified:
>
> JSON says it is undefined what happens when a client passes double
> keys. We are probably best off if we teach our parser to be strict and
> reject doubled keys in QMP as invalid.

Not bug-compatible.  Do we care?

> Hmm - can a client abuse QMP with duplicate keys to cause qemu to leak
> memory?

No.  parse_pair() inserts with qdict_put_obj(), which replaces the old
value without leaking it.

>>
>> $ qemu-system-x86_64 -qmp stdio
>> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 3},
>> "package": "v3.1.0-rc3-7-g87a45d86ed"}, "capabilities": []}}
>> {'execute':'qmp_capabilities'}
>> {"return": {}}
>> {'execute':'blockdev-add','arguments':{'driver':'null-co',
>>   'node-name':'foo','node-name':'bar'}}
>> {"return": {}}
>> {'execute':'query-named-block-nodes'}
>> {"return": [{ [...] "node-name": "bar" [...] }]}
>>
>> Another test case is iotest 229 which specifies both mode=absolute-paths
>> and mode=existing (it wants the latter).
>>
>> Max
>>

Re: [Qemu-devel] QMP accepts double dict keys

[Eric Blake]

On 12/3/18 1:48 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 12/3/18 10:30 AM, Max Reitz wrote:
>>> Hi,
>>>
>>> QMP accepts double keys in dicts without complaining.  The value it is
>>> using is apparently the last one specified:
>>
>> JSON says it is undefined what happens when a client passes double
>> keys. We are probably best off if we teach our parser to be strict and
>> reject doubled keys in QMP as invalid.
> 
> Not bug-compatible.  Do we care?

I don't think so. Such a client was already invoking undefined behavior. 
Relying on first- or last-past-the-post to win is not portable, since 
JSON parsers are allowed to use hash tables with non-deterministic 
lookups. I think erroring out is nicer than silently accepting one 
thing, especially if that might have been different than what the client 
(incorrectly) expected.  I'm not even sure that we would want a 
deprecation period.

> 
>> Hmm - can a client abuse QMP with duplicate keys to cause qemu to leak
>> memory?
> 
> No.  parse_pair() inserts with qdict_put_obj(), which replaces the old
> value without leaking it.

Good to know.

>>>
>>> Another test case is iotest 229 which specifies both mode=absolute-paths
>>> and mode=existing (it wants the latter).

We'll have to fix such broken clients, of course. If it is just our 
iotests (and not libvirt), I'm less worried about the change in behavior.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] QMP accepts double dict keys

[Daniel P. Berrangé]

On Mon, Dec 03, 2018 at 01:57:13PM -0600, Eric Blake wrote:
> On 12/3/18 1:48 PM, Markus Armbruster wrote:
> > Eric Blake <eblake@redhat.com> writes:
> > 
> > > On 12/3/18 10:30 AM, Max Reitz wrote:
> > > > Hi,
> > > > 
> > > > QMP accepts double keys in dicts without complaining.  The value it is
> > > > using is apparently the last one specified:
> > > 
> > > JSON says it is undefined what happens when a client passes double
> > > keys. We are probably best off if we teach our parser to be strict and
> > > reject doubled keys in QMP as invalid.
> > 
> > Not bug-compatible.  Do we care?
> 
> I don't think so. Such a client was already invoking undefined behavior.
> Relying on first- or last-past-the-post to win is not portable, since JSON
> parsers are allowed to use hash tables with non-deterministic lookups. I
> think erroring out is nicer than silently accepting one thing, especially if
> that might have been different than what the client (incorrectly) expected.
> I'm not even sure that we would want a deprecation period.

Erroring out immediately, without any deprecation period sounds fine
to me, as this is simply a bug fix.

IMHO the ABI/API compatibility stability only applies to things that
are intended behaviour / correct usage.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] QMP accepts double dict keys

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 12/3/18 1:48 PM, Markus Armbruster wrote:
> > Eric Blake <eblake@redhat.com> writes:
> > 
> > > On 12/3/18 10:30 AM, Max Reitz wrote:
> > > > Hi,
> > > > 
> > > > QMP accepts double keys in dicts without complaining.  The value it is
> > > > using is apparently the last one specified:
> > > 
> > > JSON says it is undefined what happens when a client passes double
> > > keys. We are probably best off if we teach our parser to be strict and
> > > reject doubled keys in QMP as invalid.
> > 
> > Not bug-compatible.  Do we care?
> 
> I don't think so. Such a client was already invoking undefined behavior.
> Relying on first- or last-past-the-post to win is not portable, since JSON
> parsers are allowed to use hash tables with non-deterministic lookups. I
> think erroring out is nicer than silently accepting one thing, especially if
> that might have been different than what the client (incorrectly) expected.
> I'm not even sure that we would want a deprecation period.

Agreed, because it's the type of thing that ends up being potentially
dangerous, since if you had some form of security check that was looking
at QMP messages it might check one version of the field and not the
other.

Dave

> > 
> > > Hmm - can a client abuse QMP with duplicate keys to cause qemu to leak
> > > memory?
> > 
> > No.  parse_pair() inserts with qdict_put_obj(), which replaces the old
> > value without leaking it.
> 
> Good to know.
> 
> > > > 
> > > > Another test case is iotest 229 which specifies both mode=absolute-paths
> > > > and mode=existing (it wants the latter).
> 
> We'll have to fix such broken clients, of course. If it is just our iotests
> (and not libvirt), I'm less worried about the change in behavior.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK