Guard against stack overflow during initial sync of bookmarks

This change adds stack overflow protection for the recursive call
to build the bookmarks tree.  The depth of the tree is limited to
200 as we do for importing bookmarks.  Anything deeper than this
is silently ignored in the sync operation.

Bug: 1093190
Change-Id: Id0cab5097aadc9068a77578ec4c39de536d3818a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246791
Commit-Queue: Chris Davis <chrdavis@microsoft.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783054}

components/sync_bookmarks/bookmark_model_merger.cc

@@ -50,6 +50,10 @@ const char kBookmarkBarTag[] = "bookmark_bar";
 const char kMobileBookmarksTag[] = "synced_bookmarks";
 const char kOtherBookmarksTag[] = "other_bookmarks";
 
+// Maximum depth to sync bookmarks tree to protect against stack overflow.
+// Keep in sync with |base::internal::kAbsoluteMaxDepth| in json_common.h.
+const size_t kMaxBookmarkTreeDepth = 200;
+
 // The value must be a list since there is a container using pointers to its
 // elements.
 using UpdatesPerParentId =
@@ -370,12 +374,19 @@ bool BookmarkModelMerger::RemoteTreeNode::UniquePositionLessThan(
 BookmarkModelMerger::RemoteTreeNode
 BookmarkModelMerger::RemoteTreeNode::BuildTree(
     UpdateResponseData update,
+    size_t max_depth,
     UpdatesPerParentId* updates_per_parent_id) {
   DCHECK(updates_per_parent_id);
 
   RemoteTreeNode node;
   node.update_ = std::move(update);
 
+  // Ensure we have not reached the maximum tree depth to guard against stack
+  // overflows.
+  if (max_depth == 0) {
+    return node;
+  }
+
   // Only folders may have descendants (ignore them otherwise). Treat
   // permanent nodes as folders explicitly.
   if (!node.update_.entity.is_folder &&
@@ -398,8 +409,8 @@ BookmarkModelMerger::RemoteTreeNode::BuildTree(
     DCHECK(IsValidBookmarkSpecifics(child_update.entity.specifics.bookmark(),
                                     child_update.entity.is_folder));
 
-    node.children_.push_back(
-        BuildTree(std::move(child_update), updates_per_parent_id));
+    node.children_.push_back(BuildTree(std::move(child_update), max_depth - 1,
+                                       updates_per_parent_id));
   }
 
   // Sort the children according to their unique position.
@@ -480,7 +491,8 @@ BookmarkModelMerger::RemoteForest BookmarkModelMerger::BuildRemoteForest(
 
     update_forest.emplace(
         server_defined_unique_tag,
-        RemoteTreeNode::BuildTree(std::move(update), &updates_per_parent_id));
+        RemoteTreeNode::BuildTree(std::move(update), kMaxBookmarkTreeDepth,
+                                  &updates_per_parent_id));
   }
 
   // All remaining entries in |updates_per_parent_id| must be unreachable from

components/sync_bookmarks/bookmark_model_merger.h

@@ -68,8 +68,10 @@ class BookmarkModelMerger {
     // |updates_per_parent_id| must not be null. All updates
     // |*updates_per_parent_id| must represent valid updates. Updates
     // corresponding from descendant nodes are moved away from
-    // |*updates_per_parent_id|.
+    // |*updates_per_parent_id|. |max_depth| is the max tree depth to sync
+    // after which content is silently ignored.
     static RemoteTreeNode BuildTree(syncer::UpdateResponseData update,
+                                    size_t max_depth,
                                     UpdatesPerParentId* updates_per_parent_id);
 
     ~RemoteTreeNode();

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/guid.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
@@ -1952,4 +1953,57 @@ TEST(BookmarkModelMergerTest, ShouldRemoveDifferentFolderDuplicatesByGUID) {
   EXPECT_EQ(bookmark_bar_node->children().front()->children().size(), 2u);
 }
 
+// This tests ensures maximum depth of the bookmark tree is not exceeded. This
+// prevents a stack overflow.
+TEST(BookmarkModelMergerTest, ShouldEnsureLimitDepthOfTree) {
+  const std::string kLocalTitle = "local";
+  const std::string kRemoteTitle = "remote";
+  const std::string folderIdPrefix = "folder_";
+  // Maximum depth to sync bookmarks tree to protect against stack overflow.
+  // This matches |kMaxBookmarkTreeDepth| in bookmark_model_merger.cc.
+  const size_t kMaxBookmarkTreeDepth = 200;
+  const size_t kRemoteUpdatesDepth = kMaxBookmarkTreeDepth + 10;
+
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  // -------- The local model --------
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* folder = bookmark_model->AddFolder(
+      /*parent=*/bookmark_bar_node, /*index=*/0,
+      base::UTF8ToUTF16(kLocalTitle));
+  ASSERT_TRUE(folder);
+
+  // -------- The remote model --------
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateBookmarkBarNodeUpdateData());
+
+  std::string parent_id = kBookmarkBarId;
+  // Create a tree with depth |kRemoteUpdatesDepth| to verify the limit of
+  // kMaxBookmarkTreeDepth is enforced.
+  for (size_t i = 1; i < kRemoteUpdatesDepth; ++i) {
+    std::string folder_id = folderIdPrefix + base::NumberToString(i);
+    updates.push_back(CreateUpdateResponseData(
+        /*server_id=*/folder_id, /*parent_id=*/parent_id, kRemoteTitle,
+        /*url=*/"",
+        /*is_folder=*/true, MakeRandomPosition()));
+    parent_id = folder_id;
+  }
+
+  ASSERT_THAT(updates.size(), Eq(kRemoteUpdatesDepth));
+
+  std::unique_ptr<SyncedBookmarkTracker> tracker =
+      SyncedBookmarkTracker::CreateEmpty(sync_pb::ModelTypeState());
+  testing::NiceMock<favicon::MockFaviconService> favicon_service;
+  BookmarkModelMerger(std::move(updates), bookmark_model.get(),
+                      &favicon_service, tracker.get())
+      .Merge();
+
+  // Check max depth hasn't been exceeded. Take into account root of the
+  // tracker and bookmark bar.
+  EXPECT_THAT(tracker->TrackedEntitiesCountForTest(),
+              Eq(kMaxBookmarkTreeDepth + 2));
+}
+
 }  // namespace sync_bookmarks