Re: [Qemu-devel] [PATCH 10/11] json-streamer: limit the maximum recursion depth and maximum token count

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/11/2011 05:16 PM, Michael Roth wrote:
>>           parser->emit(parser, parser->tokens);
>>           QDECREF(parser->tokens);
>>           parser->tokens = qlist_new();
>> +        parser->token_size = 0;
>> +    } else if (parser->token_size>  MAX_TOKEN_SIZE ||
>> +               parser->bracket_count>  MAX_NESTING ||
>> +               parser->brace_count>  MAX_NESTING) {
>> +        /* Security consideration, we limit total memory allocated 
>> per object
>> +         * and the maximum recursion depth that a message can force.
>> +         */
>> +        parser->brace_count = 0;
>> +        parser->bracket_count = 0;
>> +        parser->emit(parser, parser->tokens);
> @@ -56,6 +61,19 @@ static void json_message_process_token(JSONLexer 
> *lexer, QString *token, JSONTok
>
> Should we even bother passing this to the parser? That's a lot stuff 
> to churn on that we know isn't going to result in anything useful. If 
> there was a proper object earlier in the stream it would've been 
> emitted when brace/bracket count reached 0.
>
> I think it might be nicer to do parser->emit(parser, NULL), and fix up 
> json_parser_parse_err() to check for this and simply return NULL back 
> to qmp_dispatch_err() or whoever is calling.
>
> I think brace_count < 0 || bracket_count < 0 should get similar 
> treatment.

I think the main advantage of doing it this way is that we can test the 
maximum stack usage by just doing a simple:

(while true; do echo "{'key': "; done) | socat stdio 
unix:/path/to/qmp.sock > /dev/null

However, if we don't pass the token list to the parser, we need to know 
exactly what the maximum is and only generate that depth in order to 
test stack usage.

The parser needs to be robust against bad input so from a test 
perspective, I like the idea of passing something to the parser even if 
we know it's bad.

Regards,

Anthony Liguori