Fix several potential buffer over-read errors in JSONParser::ConsumeNumber.

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 662 matching lines @@
   if (*pos_ == '-')
     NextChar();
 
   if (!ReadInt(false)) {
     ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
     return nullptr;
   }
   end_index = index_;
 
   // The optional fraction part.
-  if (*pos_ == '.') {
-    if (!CanConsume(1)) {
-      ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
-      return nullptr;
-    }
+  if (CanConsume(1) && *pos_ == '.') {
     NextChar();
     if (!ReadInt(true)) {
       ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
       return nullptr;
     }
     end_index = index_;
   }
 
   // Optional exponent part.
-  if (*pos_ == 'e' || *pos_ == 'E') {
+  if (CanConsume(1) && (*pos_ == 'e' || *pos_ == 'E')) {
     NextChar();
-    if (*pos_ == '-' || *pos_ == '+')
+    if (!CanConsume(1)) {

[dcheng, 2017/02/24 23:05:33]

In theory, we don't need 696-697, and we can fold CanConsume(1) into 699.

I think it might be a bit more confusing to read that way though, so I'm OK with
this.

[Robert Sesek, 2017/02/24 23:07:21]

Yeah, I agree that this is easier to read than folding onto 699.

+      ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
+      return nullptr;
+    }
+    if (*pos_ == '-' || *pos_ == '+') {
       NextChar();
+    }
     if (!ReadInt(true)) {
       ReportError(JSONReader::JSON_SYNTAX_ERROR, 1);
       return nullptr;
     }
     end_index = index_;
   }
 
   // ReadInt is greedy because numbers have no easily detectable sentinel,
   // so save off where the parser should be on exit (see Consume invariant at
   // the top of the header), then make sure the next token is one which is
@@ skipping 24 matching lines @@
   double num_double;
   if (StringToDouble(num_string.as_string(), &num_double) &&
       std::isfinite(num_double)) {
     return base::MakeUnique<FundamentalValue>(num_double);
   }
 
   return nullptr;
 }
 
 bool JSONParser::ReadInt(bool allow_leading_zeros) {
   char first = *pos_;

[jdoerrie, 2017/02/27 10:17:03]

You should probably add

if (!CanConsume(1))
    return false;

here, before you dereference pos_. With the current set-up test cases 2, 6 and 8
(i.e. "2.", "43e-", and "2e+") should still trigger a buffer over-read. It's odd
that msan doesn't catch this, but it seems like that the StringPiece constructor
writes the length of string right after the payload. Using gdb I get "2.\002"
for input "2." and "43e-\004" for input "43e-". Therefore the memory past the
end technically is initialized and msan does not complain, but I don't think we
should rely on this behavior.

[Robert Sesek, 2017/02/27 19:43:37]

Yeah, I noticed that as well after I ran this through ASan (which caught it, but
MSan didn't. Can't explain that).

   int len = 0;
 
   char c = first;
   while (CanConsume(1) && IsAsciiDigit(c)) {
     c = *NextChar();
     ++len;
   }
 
   if (len == 0)
     return false;
@@ skipping 62 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