Fix potential buffer over-read errors for un-terminated JSON strings and comments.

base/json/json_parser.cc

Index: base/json/json_parser.cc
diff --git a/base/json/json_parser.cc b/base/json/json_parser.cc
index 391404f719514a3e42c20d2ad248e1157b267c6d..1938a2a83d5af7fd02f6658b7c2d110367799c97 100644
--- a/base/json/json_parser.cc
+++ b/base/json/json_parser.cc
@@ -279,27 +279,31 @@ bool JSONParser::EatComment() {
   if (*pos_ != '/' || !CanConsume(1))
     return false;
 
-  char next_char = *NextChar();
-  if (next_char == '/') {
+  NextChar();

[brettw, 2017/05/09 19:53:14]

Did you consider making NextChar() return void? It seems that this is a pretty
dangerous function to call. I'd prefer "void Advance()". Anyway, doesn't need to
block this...

[Robert Sesek, 2017/05/09 21:27:36]

Yeah, I was thinking about this because result of NextChar does seem to be
generally dangerous. I'll look at renaming and re-typing in the follow-up.

+
+  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 @@ bool JSONParser::ConsumeStringRaw(StringBuilder* out) {
     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 @@ bool JSONParser::ConsumeStringRaw(StringBuilder* out) {
         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 @@ std::unique_ptr<Value> JSONParser::ConsumeLiteral() {
     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 @@ std::unique_ptr<Value> JSONParser::ConsumeLiteral() {
     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 @@ std::unique_ptr<Value> JSONParser::ConsumeLiteral() {
     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;