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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 19 May 2010 16:43:08 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> 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.

 I know we're legal, but what's the point to offer this extension to clients?

 The main motivation behind this was to write JSON in C strings w/o the
need of repetitive escapes. This is internal to QEMU, but it's also
available to clients for no reason.

 And you know, after 0.13 we won't be able to remove it.

> > 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.

 Ok.

> > 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.

 Yes, json_message_parser_feed() returns OK.

> [IN_WHITESPACE] -> TERMINAL(JSON_SKIP)
> 
> JSON_SKIP is a terminal so once you're in that state, you go back to 
> IN_START.

 Yes, but what I'm trying to say is that when ch='\0' and you do:

     lexer->state = json_lexer[IN_START][(uint8_t)ch];

 Then 'lexer->state' becomes 0, which is what the code recognizes as ERROR.

 Again, not sure if this is an issue. Just caught my attention.

> > 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

 Ok.