8:57 a.m.
We use JSON in several external interfaces:
* QMP
* The guest agent's QMP
* QAPIfied command line options when the option argument starts with
'{'
* The block layer's pseudo-protocol "json:" (which can get embedded in
image headers)
I *think* that's all.
The JSON parser we use for these interfaces supports extensions over RFC
8259. Quoting json-lexer.c:
- Extra escape sequence in strings:
0x27 (apostrophe) is recognized after escape, too
- Single-quoted strings:
Like double-quoted strings, except they're delimited by %x27
(apostrophe) instead of %x22 (quotation mark), and can't contain
unescaped apostrophe, but can contain unescaped quotation mark.
- Interpolation, if enabled:
The lexer accepts %[A-Za-z0-9]*, and leaves rejecting invalid
ones to the parser.
Ignore interpolation; it's never enabled at external interfaces.
This leaves single-quotes strings and the escape sequence to go with
them.
I disabled them as an experiment. Some 20 iotests, a qtest and two unit
tests explode.
The unit test testing the JSON parser is of course excused.
The remaining qtest and the unit test could perhaps be dismissed as
atypical use of QEMU from C. The iotests less so, I think.
I looked at some iotest failures, and quickly found single-quoted
strings used with all external interfaces except for qemu-ga's QMP.
We could certainly tidy up the tests to stick to standard JSON.
However, the prevalence of single-quoted strings in iotests makes me
suspect that they are being used in the field as well. Deprecating the
extension is likely more trouble than it's worth.
Opinions?
9:10 a.m.
On Mon, Feb 22, 2021 at 15:57:22 +0100, Markus Armbruster wrote:
We use JSON in several external interfaces:
* QMP
* The guest agent's QMP
* QAPIfied command line options when the option argument starts with
'{'
* The block layer's pseudo-protocol "json:" (which can get embedded in
image headers)
I *think* that's all.
The JSON parser we use for these interfaces supports extensions over RFC
8259. Quoting json-lexer.c:
- Extra escape sequence in strings:
0x27 (apostrophe) is recognized after escape, too
- Single-quoted strings:
Like double-quoted strings, except they're delimited by %x27
(apostrophe) instead of %x22 (quotation mark), and can't contain
unescaped apostrophe, but can contain unescaped quotation mark.
[...]
We could certainly tidy up the tests to stick to standard JSON.
However, the prevalence of single-quoted strings in iotests makes me
suspect that they are being used in the field as well. Deprecating the
extension is likely more trouble than it's worth.
Opinions?
Any user of QEMU through libvirt will not use any of the extensions even
in cases such as QMP command pasthrough (virsh qemu-monitor-command) and
the 'json:' pseudo-protocol, as libvirt parses the provided JSON to add
sequencing for QMP passthrough, and for image chain detection in case of
'json:'. Since libvirt's JSON library (yajl) doesn't support any of
those extensions users are forced to not use them.
So on behalf of libvirt, we'd be fine with deprecation/removal of those.
9:24 a.m.
On Mon, Feb 22, 2021 at 03:57:22PM +0100, Markus Armbruster wrote:
We use JSON in several external interfaces:
* QMP
* The guest agent's QMP
* QAPIfied command line options when the option argument starts with
'{'
* The block layer's pseudo-protocol "json:" (which can get embedded in
image headers)
I *think* that's all.
The JSON parser we use for these interfaces supports extensions over RFC
8259. Quoting json-lexer.c:
- Extra escape sequence in strings:
0x27 (apostrophe) is recognized after escape, too
- Single-quoted strings:
Like double-quoted strings, except they're delimited by %x27
(apostrophe) instead of %x22 (quotation mark), and can't contain
unescaped apostrophe, but can contain unescaped quotation mark.
- Interpolation, if enabled:
The lexer accepts %[A-Za-z0-9]*, and leaves rejecting invalid
ones to the parser.
Ignore interpolation; it's never enabled at external interfaces.
This leaves single-quotes strings and the escape sequence to go with
them.
I disabled them as an experiment. Some 20 iotests, a qtest and two unit
tests explode.
The unit test testing the JSON parser is of course excused.
The remaining qtest and the unit test could perhaps be dismissed as
atypical use of QEMU from C. The iotests less so, I think.
I looked at some iotest failures, and quickly found single-quoted
strings used with all external interfaces except for qemu-ga's QMP.
We could certainly tidy up the tests to stick to standard JSON.
However, the prevalence of single-quoted strings in iotests makes me
suspect that they are being used in the field as well. Deprecating the
extension is likely more trouble than it's worth.
The shell based iotests use single quotes primarily because they're
being written in a language which lacks the concept of libraries and
and so all JSON is constructed by string substitution. Using single
quotes is convenient to avoid continually escaping double quotes.
For any other language constructing JSON documents through string
substitution is insanity, because they all have JSON libraries
available which let you construct JSON documents progamatically
without risk of introducing security flaws through malicious
substitutions.
This problem isn't unique to QEMU. Any app using JSON from the
shell will have the tedium of quote escaping. JSON is incredibly
widespread and no other apps felt it neccessary to introduce single
quoting support, because the benefit doesn't outweigh the interop
problem it introduces.
Opinions?
IMHO we should deprecate and eventually remove single quotes. We
should expect mgmt apps to be using a JSON library to talk to QEMU
in general if they are using QMP. Sure some may be using shell, but
I'd expect that to be relatively few. Adapting is tedious but not
especially hard.
It would be nice at some point in future to have the option of using
a standard JSON library in part or all of QEMU. Especially if we
ever want to be able to have parts of QEMU written in non-C language,
we don't want to re-invent a custom JSON parser as the first step,
for back compatibility.
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 :|
9:43 a.m.
On Mon, 22 Feb 2021 at 17:27, Daniel P. Berrangé <berrange(a)redhat.com>
wrote:
IMHO we should deprecate and eventually remove single quotes....
+1
If a JSON cannot be directly processed by the standard JavaScript parser,
it should not be used.
Regards,
Liviu
--
Sent from my iPad via Gmail.
11:47 a.m.
On 22/02/21 16:24, Daniel P. Berrangé wrote:
This problem isn't unique to QEMU. Any app using JSON from the
shell will have the tedium of quote escaping. JSON is incredibly
widespread and no other apps felt it neccessary to introduce single
quoting support, because the benefit doesn't outweigh the interop
problem it introduces.
The quotes were introduced for C code (and especially qtest), not for
the shell. We have something like
response = qmp("{ 'execute': 'qom-get', 'arguments': {
'path': %s, "
"'property': 'temperature' } }", id);
These are sent to QEMU as double-quoted strings (the single-quoted JSON
is parsed to get interpolation and printed back; commit 563890c7c7,
"libqtest: escape strings in QMP commands, fix leak", 2014-07-01).
However, doing the interpolation requires a parser that recognizes the
single-quoted strings.
Markus, did you rebuild the qtests after disabling single-quoted
strings? "make check-qtest-x86_64" would have rebuilt them, but I'm
confused by the results.
Paolo
11:54 a.m.
On Mon, Feb 22, 2021 at 06:47:30PM +0100, Paolo Bonzini wrote:
On 22/02/21 16:24, Daniel P. Berrangé wrote:
> This problem isn't unique to QEMU. Any app using JSON from the
> shell will have the tedium of quote escaping. JSON is incredibly
> widespread and no other apps felt it neccessary to introduce single
> quoting support, because the benefit doesn't outweigh the interop
> problem it introduces.
The quotes were introduced for C code (and especially qtest), not for the
shell. We have something like
response = qmp("{ 'execute': 'qom-get', 'arguments': {
'path': %s, "
"'property': 'temperature' } }", id);
These are sent to QEMU as double-quoted strings (the single-quoted JSON is
parsed to get interpolation and printed back; commit 563890c7c7, "libqtest:
escape strings in QMP commands, fix leak", 2014-07-01). However, doing the
interpolation requires a parser that recognizes the single-quoted strings.
IMHO this is the wrong solution to the problem. Consider the equivalent
libvirt code that uses a standard JSON library underneath and has a high
level API to serialize args into the command
qemuMonitorJSONMakeCommand("qom-get",
"s:path", id,
"s:property", "temperature");
Of course this example is reasonably easy since it is a flat set of
arguments. Nested args get slightly more complicated, but still always
preferrable to doing string interpolation IMHO.
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 :|
12:06 p.m.
On 22/02/21 18:54, Daniel P. Berrangé wrote:
> These are sent to QEMU as double-quoted strings (the
single-quoted JSON is
> parsed to get interpolation and printed back; commit 563890c7c7, "libqtest:
> escape strings in QMP commands, fix leak", 2014-07-01). However, doing the
> interpolation requires a parser that recognizes the single-quoted strings.
IMHO this is the wrong solution to the problem. Consider the equivalent
libvirt code that uses a standard JSON library underneath and has a high
level API to serialize args into the command
qemuMonitorJSONMakeCommand("qom-get",
"s:path", id,
"s:property", "temperature");
Of course this example is reasonably easy since it is a flat set of
arguments. Nested args get slightly more complicated, but still always
preferrable to doing string interpolation IMHO.
I don't disagree. I'm just stating why I wanted a clarification from Markus.
Paolo
3:31 a.m.
Daniel P. Berrangé <berrange(a)redhat.com> writes:
On Mon, Feb 22, 2021 at 06:47:30PM +0100, Paolo Bonzini wrote:
> On 22/02/21 16:24, Daniel P. Berrangé wrote:
> > This problem isn't unique to QEMU. Any app using JSON from the
> > shell will have the tedium of quote escaping. JSON is incredibly
> > widespread and no other apps felt it neccessary to introduce single
> > quoting support, because the benefit doesn't outweigh the interop
> > problem it introduces.
>
> The quotes were introduced for C code (and especially qtest), not for the
> shell. We have something like
>
> response = qmp("{ 'execute': 'qom-get', 'arguments':
{ 'path': %s, "
> "'property': 'temperature' } }", id);
>
> These are sent to QEMU as double-quoted strings (the single-quoted JSON is
> parsed to get interpolation and printed back; commit 563890c7c7, "libqtest:
> escape strings in QMP commands, fix leak", 2014-07-01). However, doing the
> interpolation requires a parser that recognizes the single-quoted strings.
IMHO this is the wrong solution to the problem. Consider the equivalent
libvirt code that uses a standard JSON library underneath and has a high
level API to serialize args into the command
qemuMonitorJSONMakeCommand("qom-get",
"s:path", id,
"s:property", "temperature");
Of course this example is reasonably easy since it is a flat set of
arguments. Nested args get slightly more complicated, but still always
preferrable to doing string interpolation IMHO.
Misunderstanding: our JSON interpolation feature is *not* string
interpolation! It interpolates *objects* into the QObject built by the
parser.
Best explained with an example. The qmp() call above internally builds
the following QObject:
QDict mapping
key "execute" to a QString for "qom-get"
key "arguments" to a QDict mapping
key "path" to a QString for @id (interpolation!)
key "property" to a QString for "temperature"
Unlike string interpolation, this is safe for any valid C string @id.
If you think like a hacker, you might try shenanigans like
"{'execute': '%s'}"
You will then find that somebody has thought like a hacker before
you[*].
The %-sequences are cleverly chosen to get some protection against
common mistakes from the compiler.
This interpolation has worked quite well for us, and I have no plans to
replace it. Doesn't mean I'm against replacing it, only that I want to
see benefits exceeding the costs.
A bigger stumbling block for replacement is our need for a streaming
interface: we feed the parser characters, and expect to be called back
when an expression is complete.
[*] Commit 16a4859921 "json: Improve safety of
qobject_from_jsonf_nofail() & friends", fixed up in commit bbc0586ced
"json: Fix % handling when not interpolating".
3:37 a.m.
Markus Armbruster <armbru(a)redhat.com> writes:
[...]
A bigger stumbling block for replacement is our need for a streaming
interface: we feed the parser characters, and expect to be called back
when an expression is complete.
Another stumbling block: check-qjson.c test case "/literals/string/utf8"
and "/literals/string/escaped". Off-the-shelf parsers flunking them
would surprise me about as much as the sun going up in the morning.
[...]
5:03 a.m.
On Tue, 23 Feb 2021 at 09:33, Markus Armbruster <armbru(a)redhat.com> wrote:
Misunderstanding: our JSON interpolation feature is *not* string
interpolation! It interpolates *objects* into the QObject built by the
parser.
Given that it's basically undocumented except in a scattered
handful of comments inside the qjson parser implementation, it's
not too surprising that people misunderstand it :-) (One surprising
feature: the parser takes ownership of the object that you pass it
via the '%p' interpolation, and will qobject_unref() it.)
-- PMM
6:43 a.m.
Peter Maydell <peter.maydell(a)linaro.org> writes:
On Tue, 23 Feb 2021 at 09:33, Markus Armbruster
<armbru(a)redhat.com> wrote:
> Misunderstanding: our JSON interpolation feature is *not* string
> interpolation! It interpolates *objects* into the QObject built by the
> parser.
Given that it's basically undocumented except in a scattered
handful of comments inside the qjson parser implementation, it's
not too surprising that people misunderstand it :-)
Yes, that's fair.
I added a fair amount of commentary, but it's heavily geared towards
maintainers, not users.
(One surprising
feature: the parser takes ownership of the object that you pass it
via the '%p' interpolation, and will qobject_unref() it.)
Yes, %p takes over the reference.
3:06 a.m.
Paolo Bonzini <pbonzini(a)redhat.com> writes:
On 22/02/21 16:24, Daniel P. Berrangé wrote:
> This problem isn't unique to QEMU. Any app using JSON from the
> shell will have the tedium of quote escaping. JSON is incredibly
> widespread and no other apps felt it neccessary to introduce single
> quoting support, because the benefit doesn't outweigh the interop
> problem it introduces.
The quotes were introduced for C code (and especially qtest), not for
the shell. We have something like
response = qmp("{ 'execute': 'qom-get', 'arguments': {
'path': %s, "
"'property': 'temperature' } }", id);
These are sent to QEMU as double-quoted strings (the single-quoted
JSON is parsed to get interpolation and printed back; commit
563890c7c7, "libqtest: escape strings in QMP commands, fix leak",
2014-07-01). However, doing the interpolation requires a parser that
recognizes the single-quoted strings.
Doing interpolation requires a parser that recognizes %-sequences.
Single quote support isn't *required*, but quite desirable to let us
avoid leaning toothpick syndrome (LTS).
Example: compare the above to
response = qmp("{ \"execute\": \"qom-get\",
\"arguments\": { \"path\": %s, "
"\"property\": \"temperature\" } }",
id);
We kept the interpolation extension out of the external interfaces, but
not the single quotes.
Markus, did you rebuild the qtests after disabling single-quoted
strings? "make check-qtest-x86_64" would have rebuilt them, but I'm
confused by the results.
I ran "make check" and looked at the failures:
* check-qjson.c
- escaped_string() covers \'. Naturally, this fails.
- escaped_string() and utf8_string() try every string twice, first in
double quotes, then in single quotes. Naturally, the latter fails.
- string_with_quotes() tests unquoted single quote in double-quoted
string, and unquoted double quote in single-quoted string.
Naturally, the latter fails.
- large_dict() and simple_whitespace() use single quotes to avoid LTS.
This is the test my "The unit test testing the JSON parser is of
course excused" referred to.
* test-qobject-input-visitor.c
* qtest/qmp-test.c
More LTS avoidance.
This is "The remaining qtest and the unit test could perhaps be
dismissed as atypical use of QEMU from C."
* tests/qemu-iotests/
Unlike the tests above, these use *external* interfaces.
In shell, we need to use double quotes to get parameter expansion. We
then use single quotes to avoid LTS.
The Python code has less excuse, I think.
Still confused?
4:59 a.m.
On 23/02/21 10:06, Markus Armbruster wrote:
> Markus, did you rebuild the qtests after disabling single-quoted
> strings? "make check-qtest-x86_64" would have rebuilt them, but I'm
> confused by the results.
I ran "make check" and looked at the failures:
Still confused?
Yes. What's the patch that you used to disable the single quotes, and
why didn't the patched parser choke on
response = qmp("{ 'execute': 'qom-get', 'arguments': {
'path': %s, "
"'property': 'temperature' } }", id);
?
Paolo
6:34 a.m.
Paolo Bonzini <pbonzini(a)redhat.com> writes:
On 23/02/21 10:06, Markus Armbruster wrote:
>> Markus, did you rebuild the qtests after disabling single-quoted
>> strings? "make check-qtest-x86_64" would have rebuilt them, but
I'm
>> confused by the results.
> I ran "make check" and looked at the failures:
> Still confused?
Yes. What's the patch that you used to disable the single quotes, and
why didn't the patched parser choke on
response = qmp("{ 'execute': 'qom-get', 'arguments': {
'path': %s, "
"'property': 'temperature' } }", id);
?
My bad! I neglected to mention that I tied "disable single-quoted
strings" to "interpolation is off" for my experiment. This is a lazy,
half-assed approximation of "internal interface".
Here's the experimental patch.
commit 57138b9d4188dd8ce1814237fe53f7131bbb3f45
Author: Markus Armbruster <armbru(a)redhat.com>
Date: Mon Feb 22 17:04:10 2021 +0100
qobject: Tie single quote extension to interpolation WIP
This makes several tests fail or hang. Some are hacked up.
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 008b326fb8..c1ddc65d96 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -150,9 +150,6 @@ static QString *parse_string(JSONParserContext *ctxt, JSONToken
*token)
case '"':
g_string_append_c(str, '"');
break;
- case '\'':
- g_string_append_c(str, '\'');
- break;
case '\\':
g_string_append_c(str, '\\');
break;
@@ -201,6 +198,12 @@ static QString *parse_string(JSONParserContext *ctxt, JSONToken
*token)
}
g_string_append(str, utf8_buf);
break;
+ case '\'':
+ if (ctxt->ap) {
+ g_string_append_c(str, '\'');
+ break;
+ }
+ /* fall through */
default:
parse_error(ctxt, token, "invalid escape sequence in string");
goto out;
diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index b93d97b995..3d4d3b484e 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -49,6 +49,11 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
case JSON_RSQUARE:
parser->bracket_count--;
break;
+ case JSON_STRING:
+ if (input->str[0] == '\"' || parser->ap) {
+ break;
+ }
+ /* fall through */
case JSON_ERROR:
error_setg(&err, "JSON parse error, stray '%s'",
input->str);
goto out_emit;
11:42 a.m.
On 22/02/21 15:57, Markus Armbruster wrote:
* The block layer's pseudo-protocol "json:" (which can
get embedded in
image headers)
If it gets embedded in image headers, I don't think we'll be able to
deprecate it ever. We'd need to keep a converter for old images, at
which point it's simpler to keep the extensions.
Paolo
12:01 p.m.
On Mon, Feb 22, 2021 at 06:42:00PM +0100, Paolo Bonzini wrote:
On 22/02/21 15:57, Markus Armbruster wrote:
> * The block layer's pseudo-protocol "json:" (which can get embedded
in
> image headers)
If it gets embedded in image headers, I don't think we'll be able to
deprecate it ever. We'd need to keep a converter for old images, at which
point it's simpler to keep the extensions.
Even if we can only use a standard JSON parser for QMP + QGA, that's a
already a significant net win long term IMHO. Both of those are security
critical components and also areas where we might want different language
impls as we increasingly use a multi-process model, so avoiding use of
extensins is good.
Even for the block layer, we don't neccessarily need to keep compat at
runtime. It could be sufficient to have an extended deprecation period
and then provide an offline helper script to re-write the qcow2 backing
store field to use " instead of ' ....assuming we actually get real
world pushback from people who really have used ' - we don't know if
there are such people yet.
We can do deprecation in a multi stage process - deprecation for everything,
then block it for QMP + QGA after 2 cycles, while still allowing it for qcow2,
and eventually block for qcow2 3 years later or something like that.
IOW, I wouldn't give up on trying to deprecate it.
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 :|
12:22 p.m.
On Mon, Feb 22, 2021 at 18:42:00 +0100, Paolo Bonzini wrote:
On 22/02/21 15:57, Markus Armbruster wrote:
> * The block layer's pseudo-protocol "json:" (which can get embedded
in
> image headers)
If it gets embedded in image headers, I don't think we'll be able to
deprecate it ever. We'd need to keep a converter for old images, at which
point it's simpler to keep the extensions.
The converter or better 'fixer' actually doesn't need to be able to
interpret the old string, just accept a new. IOW it's more of a
documentation problem, because qemu-img can already do that since it's
able to write invalid JSON without interpreting it:
$ qemu-img rebase -f qcow2 -F qcow2 -b 'json:{' -u /tmp/ble.qcow2
$ qemu-img info /tmp/ble.qcow2
image: /tmp/ble.qcow2
file format: qcow2
virtual size: 10 MiB (10485760 bytes)
disk size: 196 KiB
cluster_size: 65536
backing file: json:{
backing file format: qcow2
Format specific information:
compat: 1.1
compression type: zlib
lazy refcounts: false
refcount bits: 16
corrupt: false
12:25 p.m.
On Mon, Feb 22, 2021 at 19:22:49 +0100, Peter Krempa wrote:
On Mon, Feb 22, 2021 at 18:42:00 +0100, Paolo Bonzini wrote:
> On 22/02/21 15:57, Markus Armbruster wrote:
> > * The block layer's pseudo-protocol "json:" (which can get
embedded in
> > image headers)
>
> If it gets embedded in image headers, I don't think we'll be able to
> deprecate it ever. We'd need to keep a converter for old images, at which
> point it's simpler to keep the extensions.
The converter or better 'fixer' actually doesn't need to be able to
interpret the old string, just accept a new. IOW it's more of a
documentation problem, because qemu-img can already do that since it's
able to write invalid JSON without interpreting it:
[...]
I forgot to add that such strings would be user-originated in the first
place. The qemu-generated one are (presumably) correct JSON.
1368
days inactive
1369
days old
17 comments
6 participants
participants (6)
-
Daniel P. Berrangé -
Liviu Ionescu -
Markus Armbruster -
Paolo Bonzini -
Peter Krempa -
Peter Maydell