#3309: optimize parsing from stream (no copying of entire JSON to memory); limit maximum depth to avoid stack overflow with malicious documents (fuzzing - #3285); code cleanup

JSON/include/Poco/JSON/Parser.h

@@ -65,7 +65,7 @@ class JSON_API Parser: private ParserImpl
 {
 public:
 
-	Parser(const Handler::Ptr& pHandler = new ParseHandler, std::size_t bufSize = JSON_PARSE_BUFFER_SIZE);
+	Parser(const Handler::Ptr& pHandler = new ParseHandler);
 		/// Creates JSON Parser, using the given Handler and buffer size.
 
 	virtual ~Parser();
@@ -76,6 +76,10 @@ public:
 
 	void setAllowComments(bool comments);
 		/// Allow or disallow comments. By default, comments are not allowed.
+		///
+		/// If set to true, comments will be filtered out of the input data
+		/// before passing the JSON on to the parser. This will impact performance,
+		/// especially when reading from a std::istream.
 
 	bool getAllowComments() const;
 		/// Returns true if comments are allowed, false otherwise.
@@ -85,7 +89,11 @@ public:
 	void setAllowNullByte(bool nullByte);
 		/// Allow or disallow null byte in strings.
 		///
-		/// By default, null byte is allowed.
+		/// By default, null byte is allowed (true).
+		///
+		/// If set to false, an additional check for "\u0000" will be performed
+		/// before passing the JSON on to the parser. This will impact performance,
+		/// especially when reading from a std::istream.
 
 	bool getAllowNullByte() const;
 		/// Returns true if null byte is allowed, false otherwise.
@@ -94,6 +102,10 @@ public:
 
 	void setDepth(std::size_t depth);
 		/// Sets the allowed JSON depth.
+		///
+		/// Default maximum depth is 128. Setting this value too high
+		/// may result in a stack overflow when parsing a (malicious)
+		/// JSON document.
 
 	std::size_t getDepth() const;
 		/// Returns the allowed JSON depth.

JSON/include/Poco/JSON/ParserImpl.h

@@ -38,11 +38,9 @@ namespace JSON {
 class JSON_API ParserImpl
 {
 protected:
-	static const std::size_t JSON_PARSE_BUFFER_SIZE = 4096;
+	static const std::size_t JSON_DEFAULT_DEPTH = 128;
-	static const std::size_t JSON_PARSER_STACK_SIZE = 128;
-	static const int         JSON_UNLIMITED_DEPTH = -1;
 
-	ParserImpl(const Handler::Ptr& pHandler = new ParseHandler, std::size_t bufSize = JSON_PARSE_BUFFER_SIZE);
+	ParserImpl(const Handler::Ptr& pHandler = new ParseHandler);
 		/// Creates JSON ParserImpl, using the given Handler and buffer size.
 
 	virtual ~ParserImpl();
@@ -101,12 +99,13 @@ private:
 	void handleObject();
 	void handle();
 	void handle(const std::string& json);
+	void handle(std::istream& json);
 	void stripComments(std::string& json);
 	bool checkError();
 
 	struct json_stream* _pJSON;
 	Handler::Ptr _pHandler;
-	int          _depth;
+	std::size_t  _depth;
 	char         _decimalPoint;
 	bool         _allowNullByte;
 	bool         _allowComments;

JSON/src/ParseHandler.cpp

@@ -138,10 +138,7 @@ void ParseHandler::setValue(const Var& value)
 			_key.clear();
 		}
 	}
-	else
+	else throw JSONException("Attempt to set value on an empty stack");
-	{
-		throw JSONException("Attempt to set value on an empty stack");
-	}
 }
 
 

JSON/src/Parser.cpp

@@ -29,8 +29,8 @@ namespace Poco {
 namespace JSON {
 
 
-Parser::Parser(const Handler::Ptr& pHandler, std::size_t bufSize):
+Parser::Parser(const Handler::Ptr& pHandler):
-	ParserImpl(pHandler, bufSize)
+	ParserImpl(pHandler)
 {
 }
 

JSON/src/ParserImpl.cpp

@@ -34,10 +34,26 @@ namespace Poco {
 namespace JSON {
 
 
-ParserImpl::ParserImpl(const Handler::Ptr& pHandler, std::size_t bufSize):
+extern "C"
+{
+	static int istream_get(void* ptr)
+	{
+		std::istream* pIstr = reinterpret_cast<std::istream*>(ptr);
+		return pIstr->get();
+	}
+
+	static int istream_peek(void* ptr)
+	{
+		std::istream* pIstr = reinterpret_cast<std::istream*>(ptr);
+		return pIstr->peek();
+	}
+}
+
+
+ParserImpl::ParserImpl(const Handler::Ptr& pHandler):
 	_pJSON(new json_stream),
 	_pHandler(pHandler),
-	_depth(JSON_UNLIMITED_DEPTH),
+	_depth(JSON_DEFAULT_DEPTH),
 	_decimalPoint('.'),
 	_allowNullByte(true),
 	_allowComments(false)
@@ -68,12 +84,34 @@ void ParserImpl::handle(const std::string& json)
 		// json_open*() call, which calls internal init()
 		json_set_streaming(_pJSON, false);
 		/////////////////////////////////
-		handle(); checkError();
+		handle();
+		checkError();
 		if (JSON_DONE != json_next(_pJSON))
 			throw JSONException("Excess characters found after JSON end.");
 		json_close(_pJSON);
 	}
-	catch (std::exception&)
+	catch (...)
+	{
+		json_close(_pJSON);
+		throw;
+	}
+}
+
+
+void ParserImpl::handle(std::istream& json)
+{
+	try
+	{
+		json_open_user(_pJSON, istream_get, istream_peek, &json);
+		checkError();
+		json_set_streaming(_pJSON, false);
+		handle();
+		checkError();
+		if (JSON_DONE != json_next(_pJSON))
+			throw JSONException("Excess characters found after JSON end.");
+		json_close(_pJSON);
+	}
+	catch (...)
 	{
 		json_close(_pJSON);
 		throw;
@@ -89,17 +127,33 @@ Dynamic::Var ParserImpl::parseImpl(const std::string& json)
 		stripComments(str);
 		handle(str);
 	}
-	else handle(json);
+	else
+	{
+		handle(json);
+	}
 
 	return asVarImpl();
 }
 
 
-Dynamic::Var ParserImpl::parseImpl(std::istream& in)
+Dynamic::Var ParserImpl::parseImpl(std::istream& json)
 {
-	std::ostringstream os;
+	if (_allowComments || !_allowNullByte)
-	StreamCopier::copyStream(in, os);
+	{
-	return parseImpl(os.str());
+		std::string str;
+		Poco::StreamCopier::copyToString(json, str);
+		if (_allowComments)
+		{
+			stripComments(str);
+		}
+		handle(str);
+	}
+	else
+	{
+		handle(json);
+	}
+
+	return asVarImpl();
 }
 
 
@@ -146,7 +200,10 @@ void ParserImpl::handleArray()
 		tok = json_peek(_pJSON);
 	}
 
-	if (tok == JSON_ARRAY_END) handle();
+	if (tok == JSON_ARRAY_END)
+	{
+		handle();
+	}
 	else throw JSONException("JSON array end not found");
 }
 
@@ -162,70 +219,74 @@ void ParserImpl::handleObject()
 		tok = json_peek(_pJSON);
 	}
 
-	if (tok == JSON_OBJECT_END) handle();
+	if (tok == JSON_OBJECT_END)
+	{
+		handle();
+	}
 	else throw JSONException("JSON object end not found");
 }
 
 
 void ParserImpl::handle()
 {
+	if (json_get_depth(_pJSON) > _depth)
+		throw JSONException("Maximum depth exceeded");
+
 	enum json_type type = json_next(_pJSON);
 	switch (type)
 	{
-		case JSON_DONE:
+	case JSON_DONE:
-			return;
+		return;
-		case JSON_NULL:
+	case JSON_NULL:
-			_pHandler->null();
+		_pHandler->null();
-			break;
+		break;
-		case JSON_TRUE:
+	case JSON_TRUE:
-			if (_pHandler) _pHandler->value(true);
+		if (_pHandler) _pHandler->value(true);
-			break;
+		break;
-		case JSON_FALSE:
+	case JSON_FALSE:
-			if (_pHandler) _pHandler->value(false);
+		if (_pHandler) _pHandler->value(false);
-			break;
+		break;
-		case JSON_NUMBER:
+	case JSON_NUMBER:
+		if (_pHandler)
 		{
-			if (_pHandler)
+			std::string str(json_get_string(_pJSON, NULL));
+			if (str.find(_decimalPoint) != str.npos || str.find('e') != str.npos || str.find('E') != str.npos)
 			{
-				std::string str(json_get_string(_pJSON, NULL));
+				_pHandler->value(NumberParser::parseFloat(str));
-				if (str.find(_decimalPoint) != str.npos || str.find('e') != str.npos || str.find('E') != str.npos)
+			}
-				{
+			else
-					_pHandler->value(NumberParser::parseFloat(str));
+			{
-				}
+				Poco::Int64 val;
+				if (NumberParser::tryParse64(str, val))
+					_pHandler->value(val);
 				else
-				{
+					_pHandler->value(NumberParser::parseUnsigned64(str));
-					Poco::Int64 val;
-					if (NumberParser::tryParse64(str, val))
-						_pHandler->value(val);
-					else
-						_pHandler->value(NumberParser::parseUnsigned64(str));
-				}
 			}
-			break;
 		}
-		case JSON_STRING:
+		break;
-			if (_pHandler)
+	case JSON_STRING:
-			{
+		if (_pHandler)
-				std::size_t length = 0;
+		{
-				const char* val = json_get_string(_pJSON, &length);
+			std::size_t length = 0;
-				_pHandler->value(std::string(val, length == 0 ? 0 : length - 1)); // Decrease the length by 1 because it also contains the terminating null character
+			const char* val = json_get_string(_pJSON, &length);
-			}
+			_pHandler->value(std::string(val, length == 0 ? 0 : length - 1)); // Decrease the length by 1 because it also contains the terminating null character
-			break;
+		}
-		case JSON_OBJECT:
+		break;
-			if (_pHandler) _pHandler->startObject();
+	case JSON_OBJECT:
-			handleObject();
+		if (_pHandler) _pHandler->startObject();
-			break;
+		handleObject();
-		case JSON_OBJECT_END:
+		break;
-			if (_pHandler) _pHandler->endObject();
+	case JSON_OBJECT_END:
-			return;
+		if (_pHandler) _pHandler->endObject();
-		case JSON_ARRAY:
+		return;
-			if (_pHandler) _pHandler->startArray();
+	case JSON_ARRAY:
-			handleArray();
+		if (_pHandler) _pHandler->startArray();
-			break;
+		handleArray();
-		case JSON_ARRAY_END:
+		break;
-			if (_pHandler) _pHandler->endArray();
+	case JSON_ARRAY_END:
-			return;
+		if (_pHandler) _pHandler->endArray();
-		case JSON_ERROR:
+		return;
+	case JSON_ERROR:
 		{
 			const char* pErr = json_get_error(_pJSON);
 			std::string err(pErr ? pErr : "JSON parser error.");

JSON/testsuite/src/JSONTest.cpp

@@ -1921,13 +1921,6 @@ void JSONTest::testUnicode()
 }
 
 
-void JSONTest::testSmallBuffer()
-{
-	Poco::JSON::Parser parser(new Poco::JSON::ParseHandler(), 4);
-	std::string jsonStr = "{ \"x\" : \"123456789012345678901234567890123456789012345678901234567890\" }";
-	parser.parse(jsonStr);
-}
-
 void JSONTest::testEscape0()
 {
 	Poco::JSON::Object::Ptr json = new Poco::JSON::Object();
@@ -2261,7 +2254,6 @@ CppUnit::Test* JSONTest::suite()
 	CppUnit_addTest(pSuite, JSONTest, testInvalidUnicodeJanssonFiles);
 	CppUnit_addTest(pSuite, JSONTest, testTemplate);
 	CppUnit_addTest(pSuite, JSONTest, testUnicode);
-	CppUnit_addTest(pSuite, JSONTest, testSmallBuffer);
 	CppUnit_addTest(pSuite, JSONTest, testEscape0);
 	CppUnit_addTest(pSuite, JSONTest, testNonEscapeUnicode);
 	CppUnit_addTest(pSuite, JSONTest, testEscapeUnicode);

JSON/testsuite/src/JSONTest.h

@@ -74,7 +74,6 @@ public:
 	void testTemplate();
 	void testUnicode();
 	void testInvalidUnicodeJanssonFiles();
-	void testSmallBuffer();
 	void testEscape0();
 	void testNonEscapeUnicode();
 	void testEscapeUnicode();