Commit f0990c9d authored by Kamal Marhubi's avatar Kamal Marhubi

Make number parsing safer

This includes:
- ensuring we don't go off the end of input if it's not null-terminated
- checking for overflow and underflow
- being more careful to check that numbers match JSON lexical syntax
parent d595f0ea
...@@ -355,10 +355,44 @@ KJ_TEST("basic json decoding") { ...@@ -355,10 +355,44 @@ KJ_TEST("basic json decoding") {
MallocMessageBuilder message; MallocMessageBuilder message;
auto root = message.initRoot<JsonValue>(); auto root = message.initRoot<JsonValue>();
// TODO(soon): this should fail, JSON doesn't allow leading + KJ_EXPECT_THROW_MESSAGE("input", json.decodeRaw("z", root));
json.decodeRaw("+123", root); }
KJ_EXPECT(root.which() == JsonValue::NUMBER);
KJ_EXPECT(root.getNumber() == 123); {
MallocMessageBuilder message;
auto root = message.initRoot<JsonValue>();
// Leading + not allowed in numbers.
KJ_EXPECT_THROW_MESSAGE("Unexpected", json.decodeRaw("+123", root));
}
{
MallocMessageBuilder message;
auto root = message.initRoot<JsonValue>();
KJ_EXPECT_THROW_MESSAGE("Overflow", json.decodeRaw("1e1024", root));
}
{
MallocMessageBuilder message;
auto root = message.initRoot<JsonValue>();
KJ_EXPECT_THROW_MESSAGE("Underflow", json.decodeRaw("1e-1023", root));
}
{
MallocMessageBuilder message;
auto root = message.initRoot<JsonValue>();
KJ_EXPECT_THROW_MESSAGE("Unexpected", json.decodeRaw("[00]", root));
KJ_EXPECT_THROW_MESSAGE("Unexpected", json.decodeRaw("[01]", root));
}
{
MallocMessageBuilder message;
auto root = message.initRoot<JsonValue>();
KJ_EXPECT_THROW_MESSAGE("Expected number", json.decodeRaw("-", root));
} }
{ {
......
...@@ -21,6 +21,8 @@ ...@@ -21,6 +21,8 @@
#include "json.h" #include "json.h"
#include <cstdlib> // std::strtod #include <cstdlib> // std::strtod
#include <errno.h> // for std::strtod errors
#include <regex> // for numbers
#include <unordered_map> #include <unordered_map>
#include <capnp/orphan.h> #include <capnp/orphan.h>
#include <kj/debug.h> #include <kj/debug.h>
...@@ -395,19 +397,27 @@ public: ...@@ -395,19 +397,27 @@ public:
case '"': parseString(output); break; case '"': parseString(output); break;
case '[': parseArray(output); break; case '[': parseArray(output); break;
case '{': parseObject(output); break; case '{': parseObject(output); break;
// TODO(security): We could check for numbers more carefully instead of case '-': case '0': case '1': case '2': case '3':
// relying on strtod. case '4': case '5': case '6': case '7': case '8':
default: parseNumber(output); break; case '9': parseNumber(output); break;
default: KJ_FAIL_REQUIRE("Unexpected input in JSON message.");
} }
} }
void parseNumber(JsonValue::Builder& output) { void parseNumber(JsonValue::Builder& output) {
// TODO(someday): strtod allows leading +, while JSON grammar does not. auto numberStr = consumeNumber();
// strtod consumes leading whitespace, so we don't have to. char *endPtr;
char *numEnd;
output.setNumber(std::strtod(remaining_.begin(), &numEnd));
advanceTo(numEnd); errno = 0;
double value = std::strtod(numberStr.begin(), &endPtr);
KJ_ASSERT(endPtr != numberStr.begin(), "strtod should not fail! Is consumeNumber wrong?");
KJ_REQUIRE((value != HUGE_VAL && value != -HUGE_VAL) || errno != ERANGE,
"Overflow in JSON number.");
KJ_REQUIRE(value != 0.0 || errno != ERANGE,
"Underflow in JSON number.");
output.setNumber(value);
} }
void parseString(JsonValue::Builder& output) { void parseString(JsonValue::Builder& output) {
...@@ -581,6 +591,30 @@ public: ...@@ -581,6 +591,30 @@ public:
return kj::String(decoded.releaseAsArray()); return kj::String(decoded.releaseAsArray());
} }
kj::String consumeNumber() {
// TODO(perf): <regex> could be eliminated.
static std::regex jsonNumber(
R"(-?(0|[1-9][0-9]*)(\.[0-9]+)?((e|E)([+-])?[0-9]+)?)");
// jsonNumber matches numbers as seen in railroad diagram at http://json.org/.
std::cmatch match;
bool foundNumber = std::regex_search(
remaining_.begin(),
remaining_.end(),
match, jsonNumber,
std::regex_constants::match_continuous);
KJ_REQUIRE(foundNumber, "Expected number in JSON input.");
kj::Vector<char> number;
number.addAll(match[0].first, match[0].second);
number.add('\0');
advanceTo(match.suffix().first);
return kj::String(number.releaseAsArray());
}
// TODO(someday): This "interface" is ugly, and won't work if/when surrogates are handled. // TODO(someday): This "interface" is ugly, and won't work if/when surrogates are handled.
void unescapeAndAppend(kj::ArrayPtr<const char> hex, kj::Vector<char>& target) { void unescapeAndAppend(kj::ArrayPtr<const char> hex, kj::Vector<char>& target) {
KJ_REQUIRE(hex.size() == 4); KJ_REQUIRE(hex.size() == 4);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment