[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 17/56] json: Reject unescaped control characters
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 17/56] json: Reject unescaped control characters |
Date: |
Fri, 10 Aug 2018 16:26:24 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 08/08/2018 07:02 AM, Markus Armbruster wrote:
>> Fix the lexer to reject unescaped control characters in JSON strings,
>> in accordance with RFC 7159.
>
> Question - can this break existing QMP clients that were relying on
> this extension working?
In theory, yes.
The "extension" is undocumented. That makes it a bug.
I'm not aware of clients relying on it.
> Libvirt used to use libyajl, now it uses libjansson. So I'll check
> both of those libraries:
>
> yajl: https://github.com/lloyd/yajl/blob/master/src/yajl_encode.c#L32
>
> default:
> if ((unsigned char) str[end] < 32) {
> CharToHex(str[end], hexBuf + 4);
> escaped = hexBuf;
>
> jansson: https://github.com/akheron/jansson/blob/master/src/dump.c#L101
>
> /* mandatory escape or control char */
> if(codepoint == '\\' || codepoint == '"' || codepoint < 0x20)
>
> Okay, both libraries appear to always send control characters encoded,
> and thus were not relying on this accidental QMP extension.
>
> Are we worried about other clients?
Breakage seems unlikely to me.
>> Bonus: we now recover more nicely from unclosed strings. E.g.
>>
>> {"one: 1}\n{"two": 2}
>>
>> now recovers cleanly after the newline, where before the lexer
>> remained confused until the next unpaired double quote or lexical
>> error.
>
> On that grounds alone, I could live with this patch, even if we end up
> having to revert it later if some client was actually depending on
> sending raw control characters as part of a string.
Having to revert the patch to stay bug-compatible wouldn't be exactly
terrible.
> Reviewed-by: Eric Blake <address@hidden>
Thanks!
- Re: [Qemu-devel] [PATCH 07/56] check-qjson: Cover escaped characters more thoroughly, part 1, (continued)
- [Qemu-devel] [PATCH 08/56] check-qjson: Streamline escaped_string()'s test strings, Markus Armbruster, 2018/08/08
- [Qemu-devel] [PATCH 35/56] json: Don't create JSON_ERROR tokens that won't be used, Markus Armbruster, 2018/08/08
- [Qemu-devel] [PATCH 17/56] json: Reject unescaped control characters, Markus Armbruster, 2018/08/08
- [Qemu-devel] [PATCH 23/56] json: Leave rejecting invalid UTF-8 to parser, Markus Armbruster, 2018/08/08
- [Qemu-devel] [PATCH 18/56] json: Revamp lexer documentation, Markus Armbruster, 2018/08/08
[Qemu-devel] [PATCH 26/56] json: Simplify parse_string(), Markus Armbruster, 2018/08/08
[Qemu-devel] [PATCH 31/56] json-parser: simplify and avoid JSONParserContext allocation, Markus Armbruster, 2018/08/08