Ensure we protect against stack overflow in JSONWriter

When importing bookmarks from an html file with a deep
folder structure (>10k) we hit a stack overflow due to
the recursive calls in JSONWriter. Fix is to check we
do not exceed a max depth similar to JSONReader.

Bug: 1007626,355722

Change-Id: Iedd9d543b7168a08af1a325059860d79d4900661
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1823809
Commit-Queue: Chris Davis <chrdavis@microsoft.com>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703999}

base/BUILD.gn

@@ -318,6 +318,7 @@ jumbo_component("base") {
     "ios/scoped_critical_action.mm",
     "ios/weak_nsobject.h",
     "ios/weak_nsobject.mm",
+    "json/json_common.h",
     "json/json_file_value_serializer.cc",
     "json/json_file_value_serializer.h",
     "json/json_parser.cc",

base/json/json_common.h

+// Copyright 2019 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.
+
+#ifndef BASE_JSON_JSON_COMMON_H_
+#define BASE_JSON_JSON_COMMON_H_
+
+#include <stddef.h>
+
+#include "base/logging.h"
+#include "base/macros.h"
+
+namespace base {
+namespace internal {
+
+// Chosen to support 99.9% of documents found in the wild late 2016.
+// http://crbug.com/673263
+const size_t kAbsoluteMaxDepth = 200;
+
+// Simple class that checks for maximum recursion/stack overflow.
+class StackMarker {
+ public:
+  StackMarker(size_t max_depth, size_t* depth)
+      : max_depth_(max_depth), depth_(depth) {
+    ++(*depth_);
+    DCHECK_LE(*depth_, max_depth_);
+  }
+  ~StackMarker() { --(*depth_); }
+
+  bool IsTooDeep() const { return *depth_ >= max_depth_; }
+
+ private:
+  const size_t max_depth_;
+  size_t* const depth_;
+
+  DISALLOW_COPY_AND_ASSIGN(StackMarker);
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_JSON_JSON_COMMON_H_

base/json/json_parser.cc

@@ -26,28 +26,6 @@ namespace internal {
 namespace {
 
 const int32_t kExtendedASCIIStart = 0x80;
-
-// Simple class that checks for maximum recursion/"stack overflow."
-class StackMarker {
- public:
-  StackMarker(int max_depth, int* depth)
-      : max_depth_(max_depth), depth_(depth) {
-    ++(*depth_);
-    DCHECK_LE(*depth_, max_depth_);
-  }
-  ~StackMarker() {
-    --(*depth_);
-  }
-
-  bool IsTooDeep() const { return *depth_ >= max_depth_; }
-
- private:
-  const int max_depth_;
-  int* const depth_;
-
-  DISALLOW_COPY_AND_ASSIGN(StackMarker);
-};
-
 constexpr uint32_t kUnicodeReplacementPoint = 0xFFFD;
 
 }  // namespace
@@ -55,7 +33,7 @@ constexpr uint32_t kUnicodeReplacementPoint = 0xFFFD;
 // This is U+FFFD.
 const char kUnicodeReplacementString[] = "\xEF\xBF\xBD";
 
-JSONParser::JSONParser(int options, int max_depth)
+JSONParser::JSONParser(int options, size_t max_depth)
     : options_(options),
       max_depth_(max_depth),
       index_(0),
@@ -65,7 +43,7 @@ JSONParser::JSONParser(int options, int max_depth)
       error_code_(JSONReader::JSON_NO_ERROR),
       error_line_(0),
       error_column_(0) {
-  CHECK_LE(max_depth, JSONReader::kStackMaxDepth);
+  CHECK_LE(max_depth, kAbsoluteMaxDepth);
 }
 
 JSONParser::~JSONParser() = default;

base/json/json_parser.h

@@ -14,6 +14,7 @@
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
+#include "base/json/json_common.h"
 #include "base/json/json_reader.h"
 #include "base/macros.h"
 #include "base/optional.h"
@@ -43,7 +44,7 @@ class JSONParserTest;
 // of the next token.
 class BASE_EXPORT JSONParser {
  public:
-  JSONParser(int options, int max_depth = JSONReader::kStackMaxDepth);
+  JSONParser(int options, size_t max_depth = kAbsoluteMaxDepth);
   ~JSONParser();
 
   // Parses the input string according to the set options and returns the
@@ -215,7 +216,7 @@ class BASE_EXPORT JSONParser {
   const int options_;
 
   // Maximum depth to parse.
-  const int max_depth_;
+  const size_t max_depth_;
 
   // The input stream being parsed. Note: Not guaranteed to NUL-terminated.
   StringPiece input_;
@@ -224,7 +225,7 @@ class BASE_EXPORT JSONParser {
   int index_;
 
   // The number of times the parser has recursed (current stack depth).
-  int stack_depth_;
+  size_t stack_depth_;
 
   // The line number that the parser is at currently.
   int line_number_;

base/json/json_reader.cc

@@ -13,10 +13,6 @@
 
 namespace base {
 
-// Chosen to support 99.9% of documents found in the wild late 2016.
-// http://crbug.com/673263
-const int JSONReader::kStackMaxDepth = 200;
-
 // Values 1000 and above are used by JSONFileValueSerializer::JsonFileError.
 static_assert(JSONReader::JSON_PARSE_ERROR_COUNT < 1000,
               "JSONReader error out of bounds");
@@ -49,20 +45,22 @@ JSONReader::ValueWithError::~ValueWithError() = default;
 JSONReader::ValueWithError& JSONReader::ValueWithError::operator=(
     ValueWithError&& other) = default;
 
-JSONReader::JSONReader(int options, int max_depth)
+JSONReader::JSONReader(int options, size_t max_depth)
     : parser_(new internal::JSONParser(options, max_depth)) {}
 
 JSONReader::~JSONReader() = default;
 
 // static
-Optional<Value> JSONReader::Read(StringPiece json, int options, int max_depth) {
+Optional<Value> JSONReader::Read(StringPiece json,
+                                 int options,
+                                 size_t max_depth) {
   internal::JSONParser parser(options, max_depth);
   return parser.Parse(json);
 }
 
 std::unique_ptr<Value> JSONReader::ReadDeprecated(StringPiece json,
                                                   int options,
-                                                  int max_depth) {
+                                                  size_t max_depth) {
   Optional<Value> value = Read(json, options, max_depth);
   return value ? Value::ToUniquePtrValue(std::move(*value)) : nullptr;
 }

base/json/json_reader.h

@@ -32,6 +32,7 @@
 #include <string>
 
 #include "base/base_export.h"
+#include "base/json/json_common.h"
 #include "base/optional.h"
 #include "base/strings/string_piece.h"
 #include "base/values.h"
@@ -58,8 +59,6 @@ enum JSONParserOptions {
 
 class BASE_EXPORT JSONReader {
  public:
-  static const int kStackMaxDepth;
-
   // Error codes during parsing.
   enum JsonParseError {
     JSON_NO_ERROR = 0,
@@ -105,7 +104,8 @@ class BASE_EXPORT JSONReader {
   static const char kInputTooLarge[];
 
   // Constructs a reader.
-  JSONReader(int options = JSON_PARSE_RFC, int max_depth = kStackMaxDepth);
+  JSONReader(int options = JSON_PARSE_RFC,
+             size_t max_depth = internal::kAbsoluteMaxDepth);
 
   ~JSONReader();
 
@@ -113,16 +113,17 @@ class BASE_EXPORT JSONReader {
   // If |json| is not a properly formed JSON string, returns base::nullopt.
   static Optional<Value> Read(StringPiece json,
                               int options = JSON_PARSE_RFC,
-                              int max_depth = kStackMaxDepth);
+                              size_t max_depth = internal::kAbsoluteMaxDepth);
 
   // Deprecated. Use the Read() method above.
   // Reads and parses |json|, returning a Value.
   // If |json| is not a properly formed JSON string, returns nullptr.
   // Wrap this in base::FooValue::From() to check the Value is of type Foo and
   // convert to a FooValue at the same time.
-  static std::unique_ptr<Value> ReadDeprecated(StringPiece json,
+  static std::unique_ptr<Value> ReadDeprecated(
+      StringPiece json,
       int options = JSON_PARSE_RFC,
-                                               int max_depth = kStackMaxDepth);
+      size_t max_depth = internal::kAbsoluteMaxDepth);
 
   // Reads and parses |json| like Read(). Returns a ValueWithError, which on
   // error, will be populated with a formatted error message, an error code, and

base/json/json_writer.cc

@@ -25,19 +25,20 @@ const char kPrettyPrintLineEnding[] = "\n";
 #endif
 
 // static
-bool JSONWriter::Write(const Value& node, std::string* json) {
-  return WriteWithOptions(node, 0, json);
+bool JSONWriter::Write(const Value& node, std::string* json, size_t max_depth) {
+  return WriteWithOptions(node, 0, json, max_depth);
 }
 
 // static
 bool JSONWriter::WriteWithOptions(const Value& node,
                                   int options,
-                                  std::string* json) {
+                                  std::string* json,
+                                  size_t max_depth) {
   json->clear();
   // Is there a better way to estimate the size of the output?
   json->reserve(1024);
 
-  JSONWriter writer(options, json);
+  JSONWriter writer(options, json, max_depth);
   bool result = writer.BuildJSONString(node, 0U);
 
   if (options & OPTIONS_PRETTY_PRINT)
@@ -46,16 +47,23 @@ bool JSONWriter::WriteWithOptions(const Value& node,
   return result;
 }
 
-JSONWriter::JSONWriter(int options, std::string* json)
+JSONWriter::JSONWriter(int options, std::string* json, size_t max_depth)
     : omit_binary_values_((options & OPTIONS_OMIT_BINARY_VALUES) != 0),
       omit_double_type_preservation_(
           (options & OPTIONS_OMIT_DOUBLE_TYPE_PRESERVATION) != 0),
       pretty_print_((options & OPTIONS_PRETTY_PRINT) != 0),
-      json_string_(json) {
+      json_string_(json),
+      max_depth_(max_depth),
+      stack_depth_(0) {
   DCHECK(json);
+  CHECK_LE(max_depth, internal::kAbsoluteMaxDepth);
 }
 
 bool JSONWriter::BuildJSONString(const Value& node, size_t depth) {
+  internal::StackMarker depth_check(max_depth_, &stack_depth_);
+  if (depth_check.IsTooDeep())
+    return false;
+
   switch (node.type()) {
     case Value::Type::NONE:
       json_string_->append("null");

base/json/json_writer.h

@@ -10,6 +10,7 @@
 #include <string>
 
 #include "base/base_export.h"
+#include "base/json/json_common.h"
 #include "base/macros.h"
 
 namespace base {
@@ -42,16 +43,21 @@ class BASE_EXPORT JSONWriter {
   // TODO(tc): Should we generate json if it would be invalid json (e.g.,
   // |node| is not a DictionaryValue/ListValue or if there are inf/-inf float
   // values)? Return true on success and false on failure.
-  static bool Write(const Value& node, std::string* json);
+  static bool Write(const Value& node,
+                    std::string* json,
+                    size_t max_depth = internal::kAbsoluteMaxDepth);
 
   // Same as above but with |options| which is a bunch of JSONWriter::Options
   // bitwise ORed together. Return true on success and false on failure.
   static bool WriteWithOptions(const Value& node,
                                int options,
-                               std::string* json);
+                               std::string* json,
+                               size_t max_depth = internal::kAbsoluteMaxDepth);
 
  private:
-  JSONWriter(int options, std::string* json);
+  JSONWriter(int options,
+             std::string* json,
+             size_t max_depth = internal::kAbsoluteMaxDepth);
 
   // Called recursively to build the JSON string. When completed,
   // |json_string_| will contain the JSON.
@@ -67,6 +73,12 @@ class BASE_EXPORT JSONWriter {
   // Where we write JSON data as we generate it.
   std::string* json_string_;
 
+  // Maximum depth to write.
+  const size_t max_depth_;
+
+  // The number of times the writer has recursed (current stack depth).
+  size_t stack_depth_;
+
   DISALLOW_COPY_AND_ASSIGN(JSONWriter);
 };
 

base/json/json_writer_unittest.cc

@@ -154,4 +154,36 @@ TEST(JSONWriterTest, DoublesAsInts) {
   EXPECT_EQ("10000000000", output_js);
 }
 
+TEST(JSONWriterTest, StackOverflow) {
+  std::string output_js;
+  ListValue deep_list;
+  ListValue* next_list = &deep_list;
+
+  const size_t max_depth = 100000;
+
+  for (size_t i = 0; i < max_depth; ++i) {
+    ListValue inner_list;
+    next_list->Append(std::move(inner_list));
+    next_list->GetList(0, &next_list);
+  }
+
+  EXPECT_FALSE(JSONWriter::Write(deep_list, &output_js));
+  EXPECT_FALSE(JSONWriter::WriteWithOptions(
+      deep_list, JSONWriter::OPTIONS_PRETTY_PRINT, &output_js));
+
+  // We cannot just let deep_list tear down since it
+  // would cause a stack overflow. Therefore, we tear
+  // down from the inner lists outwards safely.
+  const size_t step = 200;
+  for (size_t i = max_depth - step; i > 0; i -= step) {
+    next_list = &deep_list;
+    for (size_t curr_depth = 0; curr_depth < i && next_list; ++curr_depth) {
+      if (!next_list->GetList(0, &next_list))
+        next_list = nullptr;
+    }
+    if (next_list)
+      next_list->Remove(0, nullptr);
+  }
+}
+
 }  // namespace base

components/prefs/json_pref_store.cc

@@ -54,6 +54,13 @@ namespace {
 // Some extensions we'll tack on to copies of the Preferences files.
 const base::FilePath::CharType kBadExtension[] = FILE_PATH_LITERAL("bad");
 
+bool BackupPrefsFile(const base::FilePath& path) {
+  const base::FilePath bad = path.ReplaceExtension(kBadExtension);
+  const bool bad_existed = base::PathExists(bad);
+  base::Move(path, bad);
+  return bad_existed;
+}
+
 PersistentPrefStore::PrefReadError HandleReadErrors(
     const base::Value* value,
     const base::FilePath& path,
@@ -78,13 +85,10 @@ PersistentPrefStore::PrefReadError HandleReadErrors(
         // We keep the old file for possible support and debugging assistance
         // as well as to detect if they're seeing these errors repeatedly.
         // TODO(erikkay) Instead, use the last known good file.
-        base::FilePath bad = path.ReplaceExtension(kBadExtension);
-
         // If they've ever had a parse error before, put them in another bucket.
         // TODO(erikkay) if we keep this error checking for very long, we may
         // want to differentiate between recent and long ago errors.
-        bool bad_existed = base::PathExists(bad);
-        base::Move(path, bad);
+        const bool bad_existed = BackupPrefsFile(path);
         return bad_existed ? PersistentPrefStore::PREF_READ_ERROR_JSON_REPEAT
                            : PersistentPrefStore::PREF_READ_ERROR_JSON_PARSE;
     }
@@ -476,8 +480,16 @@ bool JsonPrefStore::SerializeData(std::string* output) {
   // readable prefs for debugging purposes, you can dump your prefs into any
   // command-line or online JSON pretty printing tool.
   serializer.set_pretty_print(false);
-  bool success = serializer.Serialize(*prefs_);
-  DCHECK(success);
+  const bool success = serializer.Serialize(*prefs_);
+  if (!success) {
+    // Failed to serialize prefs file. Backup the existing prefs file and
+    // reset our existing prefs.
+    BackupPrefsFile(path_);
+    CHECK(false) << "Failed to serialize preferences : " << path_
+                 << "\nBacked up under "
+                 << path_.ReplaceExtension(kBadExtension);
+    prefs_.reset(new base::DictionaryValue());
+  }
   return success;
 }