Fix potential buffer over-read errors for un-terminated JSON strings and comments.
BUG=698693
TEST=base_unittests --gtest_filter=JSON* under MSan
Review-Url: https://codereview.chromium.org/2859513002
Cr-Commit-Position: refs/heads/master@{#470555}
CrOS-Libchrome-Original-Commit: 20abc56a850ed875b6be6d5ed2f8eeafeca5371a
diff --git a/base/json/json_parser.cc b/base/json/json_parser.cc
index 391404f..1938a2a 100644
--- a/base/json/json_parser.cc
+++ b/base/json/json_parser.cc
@@ -279,27 +279,31 @@
if (*pos_ != '/' || !CanConsume(1))
return false;
- char next_char = *NextChar();
- if (next_char == '/') {
+ NextChar();
+
+ if (!CanConsume(1))
+ return false;
+
+ if (*pos_ == '/') {
// Single line comment, read to newline.
while (CanConsume(1)) {
- next_char = *NextChar();
- if (next_char == '\n' || next_char == '\r')
+ if (*pos_ == '\n' || *pos_ == '\r')
return true;
+ NextChar();
}
- } else if (next_char == '*') {
+ } else if (*pos_ == '*') {
char previous_char = '\0';
// Block comment, read until end marker.
while (CanConsume(1)) {
- next_char = *NextChar();
- if (previous_char == '*' && next_char == '/') {
+ if (previous_char == '*' && *pos_ == '/') {
// EatWhitespaceAndComments will inspect pos_, which will still be on
// the last / of the comment, so advance once more (which may also be
// end of input).
NextChar();
return true;
}
- previous_char = next_char;
+ previous_char = *pos_;
+ NextChar();
}
// If the comment is unterminated, GetNextToken will report T_END_OF_INPUT.
@@ -454,15 +458,29 @@
return false;
}
+ // Strings are at minimum two characters: the surrounding double quotes.
+ if (!CanConsume(2)) {
+ ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
+ return false;
+ }
+
// StringBuilder will internally build a StringPiece unless a UTF-16
// conversion occurs, at which point it will perform a copy into a
// std::string.
StringBuilder string(NextChar());
+ // Handle the empty string case early.
+ if (*pos_ == '"') {
+ *out = std::move(string);
+ return true;
+ }
+
int length = end_pos_ - start_pos_;
int32_t next_char = 0;
- while (CanConsume(1)) {
+ // There must always be at least two characters left in the stream: the next
+ // string character and the terminating closing quote.
+ while (CanConsume(2)) {
int start_index = index_;
pos_ = start_pos_ + index_; // CBU8_NEXT is postcrement.
CBU8_NEXT(start_pos_, index_, length, next_char);
@@ -502,12 +520,18 @@
return false;
}
- switch (*NextChar()) {
+ NextChar();
+ if (!CanConsume(1)) {
+ ReportError(JSONReader::JSON_INVALID_ESCAPE, 0);
+ return false;
+ }
+
+ switch (*pos_) {
// Allowed esape sequences:
case 'x': { // UTF-8 sequence.
// UTF-8 \x escape sequences are not allowed in the spec, but they
// are supported here for backwards-compatiblity with the old parser.
- if (!CanConsume(2)) {
+ if (!CanConsume(3)) {
ReportError(JSONReader::JSON_INVALID_ESCAPE, 1);
return false;
}
@@ -777,7 +801,7 @@
case 't': {
const char kTrueLiteral[] = "true";
const int kTrueLen = static_cast<int>(strlen(kTrueLiteral));
- if (!CanConsume(kTrueLen - 1) ||
+ if (!CanConsume(kTrueLen) ||
!StringsAreEqual(pos_, kTrueLiteral, kTrueLen)) {
ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
return nullptr;
@@ -788,7 +812,7 @@
case 'f': {
const char kFalseLiteral[] = "false";
const int kFalseLen = static_cast<int>(strlen(kFalseLiteral));
- if (!CanConsume(kFalseLen - 1) ||
+ if (!CanConsume(kFalseLen) ||
!StringsAreEqual(pos_, kFalseLiteral, kFalseLen)) {
ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
return nullptr;
@@ -799,7 +823,7 @@
case 'n': {
const char kNullLiteral[] = "null";
const int kNullLen = static_cast<int>(strlen(kNullLiteral));
- if (!CanConsume(kNullLen - 1) ||
+ if (!CanConsume(kNullLen) ||
!StringsAreEqual(pos_, kNullLiteral, kNullLen)) {
ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
return nullptr;
diff --git a/base/json/json_parser_unittest.cc b/base/json/json_parser_unittest.cc
index e3f635b..dfbe77d 100644
--- a/base/json/json_parser_unittest.cc
+++ b/base/json/json_parser_unittest.cc
@@ -28,6 +28,17 @@
return parser;
}
+ // MSan will do a better job detecting over-read errors if the input is
+ // not nul-terminated on the heap. This will copy |input| to a new buffer
+ // owned by |owner|, returning a StringPiece to |owner|.
+ StringPiece MakeNotNullTerminatedInput(const char* input,
+ std::unique_ptr<char[]>* owner) {
+ size_t str_len = strlen(input);
+ owner->reset(new char[str_len]);
+ memcpy(owner->get(), input, str_len);
+ return StringPiece(owner->get(), str_len);
+ }
+
void TestLastThree(JSONParser* parser) {
EXPECT_EQ(',', *parser->NextChar());
EXPECT_EQ('|', *parser->NextChar());
@@ -367,14 +378,11 @@
auto test_case = kCases[i];
SCOPED_TRACE(StringPrintf("case %u: \"%s\"", i, test_case.input));
- // MSan will do a better job detecting over-read errors if the input is
- // not nul-terminated on the heap.
- size_t str_len = strlen(test_case.input);
- auto non_nul_termianted = MakeUnique<char[]>(str_len);
- memcpy(non_nul_termianted.get(), test_case.input, str_len);
+ std::unique_ptr<char[]> input_owner;
+ StringPiece input =
+ MakeNotNullTerminatedInput(test_case.input, &input_owner);
- StringPiece string_piece(non_nul_termianted.get(), str_len);
- std::unique_ptr<Value> result = JSONReader::Read(string_piece);
+ std::unique_ptr<Value> result = JSONReader::Read(input);
if (test_case.parse_success) {
EXPECT_TRUE(result);
} else {
@@ -390,5 +398,34 @@
}
}
+TEST_F(JSONParserTest, UnterminatedInputs) {
+ const char* kCases[] = {
+ // clang-format off
+ "/",
+ "//",
+ "/*",
+ "\"xxxxxx",
+ "\"",
+ "{ ",
+ "[\t",
+ "tru",
+ "fals",
+ "nul",
+ "\"\\x2",
+ "\"\\u123",
+ // clang-format on
+ };
+
+ for (unsigned int i = 0; i < arraysize(kCases); ++i) {
+ auto* test_case = kCases[i];
+ SCOPED_TRACE(StringPrintf("case %u: \"%s\"", i, test_case));
+
+ std::unique_ptr<char[]> input_owner;
+ StringPiece input = MakeNotNullTerminatedInput(test_case, &input_owner);
+
+ EXPECT_FALSE(JSONReader::Read(input));
+ }
+}
+
} // namespace internal
} // namespace base