From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50361 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PyQMG-0005x0-O7 for qemu-devel@nongnu.org; Sat, 12 Mar 2011 10:03:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PyQMF-0005Vi-MQ for qemu-devel@nongnu.org; Sat, 12 Mar 2011 10:03:56 -0500 Received: from mail-yw0-f45.google.com ([209.85.213.45]:33941) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PyQMF-0005VY-Jv for qemu-devel@nongnu.org; Sat, 12 Mar 2011 10:03:55 -0500 Received: by ywl41 with SMTP id 41so1864859ywl.4 for ; Sat, 12 Mar 2011 07:03:55 -0800 (PST) Message-ID: <4D7B8B58.5050903@codemonkey.ws> Date: Sat, 12 Mar 2011 09:03:52 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 10/11] json-streamer: limit the maximum recursion depth and maximum token count References: <1299877249-13433-1-git-send-email-aliguori@us.ibm.com> <1299877249-13433-11-git-send-email-aliguori@us.ibm.com> <4D7AAD33.1000804@linux.vnet.ibm.com> In-Reply-To: <4D7AAD33.1000804@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: Paolo Bonzini , Markus Armbruster , qemu-devel@nongnu.org, Michael D Roth , Luiz Capitulino 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