Re: [Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Anthony Liguori]

On 05/19/2010 04:15 PM, Luiz Capitulino wrote:
>   Hi Anthony,
>
>   While investigating a QMP bug reported by a user, I've found a few issues
> in our parser/lexer.
>
>   The patches in this series fix the problems I was able to solve, but we
> still have the following issues:
>
> 1. Our 'private extension' is open to the public
>
>     Eg. The following input issued by a client is valid:
>
>     { 'execute': 'query-pci' }
>
>     I don't think it's a good idea to have clients relying on this kind of
>     JSON extension.
>
>     To fix this we could add a 'extension' flag to JSONLexer and set it to
>     nonzero in internal functions (eg. qobject_from_jsonf()), of course that
>     the lexer code should handle this too.
>    

The JSON specification explicitly says:

"A JSON parser transforms a JSON text into another representation. A 
JSON parser MUST accept all texts that conform to the JSON grammar.  A 
JSON parser MAY accept non-JSON forms or extensions."

IOW, we're under no obligation to reject extensions and I can't think of 
a reason why we should.

> 2. QMP doesn't check the return of json_message_parser_feed()
>
>     Which means we don't handle JSON syntax errors. While the fix might seem
>     trivial (ie. just return an error!), I'm not sure what's the best way
>     to handle this, because the streamer seems to return multiple errors for
>     the same input string.
>
>     For example, this input:
>
>     { "execute": yy_uu }
>
>     Seems to return an error for each bad character (yy_uu), shouldn't it
>     return only once and stop processing the whole string?
>    

It probably should kill the connection.

> 3. The lexer enter in ERROR state when processing is done
>
>     Not sure whether this is an issue, but I found it while reviewing the code
>     and maybe this is related with item 2 above.
>
>     When json_lexer_feed_char() is finished scanning a string, (ie. ch='\0')
>     the JSON_SKIP clause will set lexer->state to ERROR as there's no entry
>     for '\0' in the IN_START array.
>
>     Shouldn't we have a LEXER_DONE or something like it instead?
>    

No, you must have malformed input if an error occurs.

[IN_WHITESPACE] -> TERMINAL(JSON_SKIP)

JSON_SKIP is a terminal so once you're in that state, you go back to 
IN_START.

> 4. Lexer expects a 'terminal' char to process a token
>
>     Which means clients must send a sort of end of line char, so that we
>     process their input.
>
>     Maybe I'm missing something here, but I thought that the whole point of
>     writing our own parser was to avoid this.
>    

If the lexer gets:

"abc"

It has no way of knowing if that's a token or if we're going to get:

"abcd"

As a token.  You can fix this in two ways.  You can either flush() the 
lexer to significant end of input or you can wait until there's some 
other valid symbol to cause the previous symbol to be emitted.

IOW, a client either needs to: 1) send the request and follow it with a 
newline or some form of whitespace or 2) close the connection to flush 
the request

Regards,

Anthony Liguori