Avoid memory corruption in sessions sync

This CL is premised on the theory that the memory corruption and related
crashes are due to invalid input data being fed into the sessions sync
code.  See the linked bug for more details.

Adds two tests that expose the scenario that is believed to be the cause
of the bug.  If checked in on their own, they would crash during
destruction of the SyncedSessionTracker.

Adds a CHECK to prevent the SyncedSessionsTracker from getting in to
a bad state.  The goal of this CHECK is to ensure that all crashes
caused by misuse of the tracker cause a crash immediately, rather than
corrupting the memory allocator's internal data structures and possibly
causing crashes in unrelated code.  The newly added tests would trigger
this CHECK, if not for the last component of this CL.

Adds a filter for incoming sync_pb::SessionHeader values.  Before acting
on the session, the SessionsSyncManager will now verify that the header
does not contain any duplicate tab IDs.  If verification fails, the
header will be ignored.  This part of the CL allows the new tests to
pass.

BUG=360822

Review URL: https://codereview.chromium.org/495593003

Cr-Commit-Position: refs/heads/master@{#291158}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@291158 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/glue/synced_session_tracker.cc

@@ -286,8 +286,20 @@ void SyncedSessionTracker::PutTabInWindow(const std::string& session_tag,
   // collected eventually.
   SessionTab* tab_ptr = GetTabImpl(
       session_tag, tab_id, TabNodePool::kInvalidTabNodeID);
+
+  // It's up to the caller to ensure this never happens.  Tabs should not
+  // belong to more than one window or appear twice within the same window.
+  //
+  // If this condition were violated, we would double-free during shutdown.
+  // That could cause all sorts of hard to diagnose crashes, possibly in code
+  // far away from here.  We crash early to avoid this.
+  //
+  // See http://crbug.com/360822.
+  CHECK(!synced_tab_map_[session_tag][tab_id].owned);
+
   unmapped_tabs_.erase(tab_ptr);
   synced_tab_map_[session_tag][tab_id].owned = true;
+
   tab_ptr->window_id.set_id(window_id);
   DVLOG(1) << "  - tab " << tab_id << " added to window "<< window_id;
   DCHECK(GetSession(session_tag)->windows.find(window_id) !=

chrome/browser/sync/sessions/sessions_sync_manager.cc

@@ -338,6 +338,24 @@ void SessionsSyncManager::RebuildAssociations() {
       syncer::SESSIONS, data, processor.Pass(), error_handler.Pass());
 }
 
+bool SessionsSyncManager::IsValidSessionHeader(
+    const sync_pb::SessionHeader& header) {
+  // Verify that tab IDs appear only once within a session.
+  // Intended to prevent http://crbug.com/360822.
+  std::set<int> session_tab_ids;
+  for (int i = 0; i < header.window_size(); ++i) {
+    const sync_pb::SessionWindow& window = header.window(i);
+    for (int j = 0; j < window.tab_size(); ++j) {
+      const int tab_id = window.tab(j);
+      bool success = session_tab_ids.insert(tab_id).second;
+      if (!success)
+        return false;
+    }
+  }
+
+  return true;
+}
+
 void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
   const content::NavigationEntry* entry = modified_tab->GetActiveEntry();
   if (!modified_tab->IsBeingDestroyed() &&
@@ -589,6 +607,12 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(
     // Header data contains window information and ordered tab id's for each
     // window.
 
+    if (!IsValidSessionHeader(specifics.header())) {
+      LOG(WARNING) << "Ignoring foreign session node with invalid header "
+                   << "and tag " << foreign_session_tag << ".";
+      return;
+    }
+
     // Load (or create) the SyncedSession object for this client.
     const sync_pb::SessionHeader& header = specifics.header();
     PopulateSessionHeaderFromSpecifics(header,

chrome/browser/sync/sessions/sessions_sync_manager.h

@@ -323,6 +323,10 @@ class SessionsSyncManager : public syncer::SyncableService,
   // See |local_tab_pool_out_of_sync_|.
   void RebuildAssociations();
 
+  // Validates the content of a SessionHeader protobuf.
+  // Returns false if validation fails.
+  static bool IsValidSessionHeader(const sync_pb::SessionHeader& header);
+
   // Mapping of current open (local) tabs to their sync identifiers.
   TabLinksMap local_tab_map_;
 

chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc

@@ -1930,4 +1930,78 @@ TEST_F(SessionsSyncManagerTest, NotifiedOfRefresh) {
 }
 #endif  // defined(OS_ANDROID) || defined(OS_IOS)
 
+// Tests receipt of duplicate tab IDs in the same window.  This should never
+// happen, but we want to make sure the client won't do anything bad if it does
+// receive such garbage input data.
+TEST_F(SessionsSyncManagerTest, ReceiveDuplicateTabInSameWindow) {
+  std::string tag = "tag1";
+
+  // Reuse tab ID 10 in an attempt to trigger bad behavior.
+  SessionID::id_type n1[] = {5, 10, 10, 17};
+  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<sync_pb::SessionSpecifics> tabs1;
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(tag, tab_list1, &tabs1));
+
+  // Set up initial data.
+  syncer::SyncDataList initial_data;
+  sync_pb::EntitySpecifics entity;
+  entity.mutable_session()->CopyFrom(meta);
+  initial_data.push_back(SyncData::CreateRemoteData(
+      1,
+      entity,
+      base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create()));
+  AddTabsToSyncDataList(tabs1, &initial_data);
+
+  syncer::SyncChangeList output;
+  InitWithSyncDataTakeOutput(initial_data, &output);
+}
+
+// Tests receipt of duplicate tab IDs for the same session.  The duplicate tab
+// ID is present in two different windows.  A client can't be expected to do
+// anything reasonable with this input, but we can expect that it doesn't
+// crash.
+TEST_F(SessionsSyncManagerTest, ReceiveDuplicateTabInOtherWindow) {
+  std::string tag = "tag1";
+
+  SessionID::id_type n1[] = {5, 10, 17};
+  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<sync_pb::SessionSpecifics> tabs1;
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(tag, tab_list1, &tabs1));
+
+  // Add a second window.  Tab ID 10 is a duplicate.
+  SessionID::id_type n2[] = {10, 18, 20};
+  std::vector<SessionID::id_type> tab_list2(n2, n2 + arraysize(n2));
+  helper()->AddWindowSpecifics(1, tab_list2, &meta);
+
+  // Set up initial data.
+  syncer::SyncDataList initial_data;
+  sync_pb::EntitySpecifics entity;
+  entity.mutable_session()->CopyFrom(meta);
+  initial_data.push_back(SyncData::CreateRemoteData(
+      1,
+      entity,
+      base::Time(),
+      syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create()));
+  AddTabsToSyncDataList(tabs1, &initial_data);
+
+  for (size_t i = 0; i < tab_list2.size(); ++i) {
+    sync_pb::EntitySpecifics entity;
+    helper()->BuildTabSpecifics(tag, 0, tab_list2[i], entity.mutable_session());
+    initial_data.push_back(SyncData::CreateRemoteData(
+        i + 10,
+        entity,
+        base::Time(),
+        syncer::AttachmentIdList(),
+        syncer::AttachmentServiceProxyForTest::Create()));
+  }
+
+  syncer::SyncChangeList output;
+  InitWithSyncDataTakeOutput(initial_data, &output);
+}
+
 }  // namespace browser_sync