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

base/json/json_parser.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/json/json_parser.h"
 
 #include <cmath>
 #include <utility>
 
 #include "base/logging.h"
@@ skipping 261 matching lines @@
       default:
         return;
     }
   }
 }
 
 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.
   }
 
   return false;
 }
 
 std::unique_ptr<Value> JSONParser::ParseNextToken() {
   return ParseToken(GetNextToken());
@@ skipping 134 matching lines @@
 
   return base::MakeUnique<Value>(string.DestructiveAsString());
 }
 
 bool JSONParser::ConsumeStringRaw(StringBuilder* out) {
   if (*pos_ != '"') {
     ReportError(JSONReader::JSON_UNEXPECTED_TOKEN, 1);
     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);
     if (next_char < 0 || !IsValidCharacter(next_char)) {
       if ((options_ & JSON_REPLACE_INVALID_CHARACTERS) == 0) {
         ReportError(JSONReader::JSON_UNSUPPORTED_ENCODING, 1);
         return false;
       }
       CBU8_NEXT(start_pos_, start_index, length, next_char);
       string.Convert();
@@ skipping 19 matching lines @@
       // (either by combining the two characters of an encoded escape sequence,
       // or with a UTF conversion), so using StringPiece isn't possible -- force
       // a conversion.
       string.Convert();
 
       if (!CanConsume(1)) {
         ReportError(JSONReader::JSON_INVALID_ESCAPE, 0);
         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;
           }
 
           int hex_digit = 0;
           if (!HexStringToInt(StringPiece(NextChar(), 2), &hex_digit) ||
               !IsValidCharacter(hex_digit)) {
             ReportError(JSONReader::JSON_INVALID_ESCAPE, -1);
             return false;
           }
@@ skipping 249 matching lines @@
     return false;
 
   return true;
 }
 
 std::unique_ptr<Value> JSONParser::ConsumeLiteral() {
   switch (*pos_) {
     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;
       }
       NextNChars(kTrueLen - 1);
       return base::MakeUnique<Value>(true);
     }
     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;
       }
       NextNChars(kFalseLen - 1);
       return base::MakeUnique<Value>(false);
     }
     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;
       }
       NextNChars(kNullLen - 1);
       return MakeUnique<Value>();
     }
     default:
       ReportError(JSONReader::JSON_UNEXPECTED_TOKEN, 1);
       return nullptr;
@@ skipping 17 matching lines @@
                                            const std::string& description) {
   if (line || column) {
     return StringPrintf("Line: %i, column: %i, %s",
         line, column, description.c_str());
   }
   return description;
 }
 
 }  // namespace internal
 }  // namespace base