[Sync] Address use-after-free in Directory::InsertEntry

Directory::InsertEntry takes pointer to EntryKernel and inserts owning pointer
into methandles_mapo with WrapUnique. The object is still owned by unique_ptr in
ModelNeutralMutableEntry ctor. If one if the steps inside InsertEntry fails
ModelNeutralMutableEntry will not release unique_ptr which will cause object to
be freed while metahandles map still has entry pointing to it.

I changed InsertEntry to pass ownint pointer to EntryKernel. Caller is free to
stash non-owning pointer, but has to reset it to nullptr if InsertEntry fails.

I refactored couple of functions in Directory to be more strict with pointers to
EntryKernel. Particularly DeleteEntry should use entry found in metahandles_map,
not the entry passed as an argument to remove entry from different indices. It
shouldn't matter in terms of correctness, but makes it easier to reason about
the logic.

BUG=705704
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2844333003
Cr-Commit-Position: refs/heads/master@{#468102}

components/sync/syncable/directory.cc

@@ -354,47 +354,50 @@ int Directory::GetPositionIndex(BaseTransaction* trans,
   return std::distance(siblings->begin(), it);
 }
 
-bool Directory::InsertEntry(BaseWriteTransaction* trans, EntryKernel* entry) {
+bool Directory::InsertEntry(BaseWriteTransaction* trans,
+                            std::unique_ptr<EntryKernel> entry) {
   ScopedKernelLock lock(this);
-  return InsertEntry(lock, trans, entry);
+  return InsertEntry(lock, trans, std::move(entry));
 }
 
 bool Directory::InsertEntry(const ScopedKernelLock& lock,
                             BaseWriteTransaction* trans,
-                            EntryKernel* entry) {
+                            std::unique_ptr<EntryKernel> entry) {
   if (!SyncAssert(nullptr != entry, FROM_HERE, "Entry is null", trans))
     return false;
+  EntryKernel* entry_ptr = entry.get();
 
   static const char error[] = "Entry already in memory index.";
 
   if (!SyncAssert(kernel_->metahandles_map
-                      .insert(std::make_pair(entry->ref(META_HANDLE),
-                                             base::WrapUnique(entry)))
+                      .insert(std::make_pair(entry_ptr->ref(META_HANDLE),
+                                             std::move(entry)))
                       .second,
                   FROM_HERE, error, trans)) {
     return false;
   }
   if (!SyncAssert(
-          kernel_->ids_map.insert(std::make_pair(entry->ref(ID).value(), entry))
+          kernel_->ids_map
+              .insert(std::make_pair(entry_ptr->ref(ID).value(), entry_ptr))
               .second,
           FROM_HERE, error, trans)) {
     return false;
   }
-  if (ParentChildIndex::ShouldInclude(entry)) {
-    if (!SyncAssert(kernel_->parent_child_index.Insert(entry), FROM_HERE, error,
-                    trans)) {
+  if (ParentChildIndex::ShouldInclude(entry_ptr)) {
+    if (!SyncAssert(kernel_->parent_child_index.Insert(entry_ptr), FROM_HERE,
+                    error, trans)) {
       return false;
     }
   }
-  AddToAttachmentIndex(lock, entry->ref(META_HANDLE),
-                       entry->ref(ATTACHMENT_METADATA));
+  AddToAttachmentIndex(lock, entry_ptr->ref(META_HANDLE),
+                       entry_ptr->ref(ATTACHMENT_METADATA));
 
   // Should NEVER be created with a client tag or server tag.
-  if (!SyncAssert(entry->ref(UNIQUE_SERVER_TAG).empty(), FROM_HERE,
+  if (!SyncAssert(entry_ptr->ref(UNIQUE_SERVER_TAG).empty(), FROM_HERE,
                   "Server tag should be empty", trans)) {
     return false;
   }
-  if (!SyncAssert(entry->ref(UNIQUE_CLIENT_TAG).empty(), FROM_HERE,
+  if (!SyncAssert(entry_ptr->ref(UNIQUE_CLIENT_TAG).empty(), FROM_HERE,
                   "Client tag should be empty", trans))
     return false;
 
@@ -617,17 +620,14 @@ bool Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) {
        ++i) {
     MetahandlesMap::iterator found =
         kernel_->metahandles_map.find((*i)->ref(META_HANDLE));
-    EntryKernel* entry =
-        (found == kernel_->metahandles_map.end() ? nullptr
-                                                 : found->second.get());
-    if (entry && SafeToPurgeFromMemory(&trans, entry)) {
+    if (found != kernel_->metahandles_map.end() &&
+        SafeToPurgeFromMemory(&trans, found->second.get())) {
       // We now drop deleted metahandles that are up to date on both the client
       // and the server.
-      std::unique_ptr<EntryKernel> unique_entry = std::move(found->second);
+      std::unique_ptr<EntryKernel> entry = std::move(found->second);
 
       size_t num_erased = 0;
-      num_erased = kernel_->metahandles_map.erase(entry->ref(META_HANDLE));
-      DCHECK_EQ(1u, num_erased);
+      kernel_->metahandles_map.erase(found);
       num_erased = kernel_->ids_map.erase(entry->ref(ID).value());
       DCHECK_EQ(1u, num_erased);
       if (!entry->ref(UNIQUE_SERVER_TAG).empty()) {
@@ -640,8 +640,8 @@ bool Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) {
             kernel_->client_tags_map.erase(entry->ref(UNIQUE_CLIENT_TAG));
         DCHECK_EQ(1u, num_erased);
       }
-      if (!SyncAssert(!kernel_->parent_child_index.Contains(entry), FROM_HERE,
-                      "Deleted entry still present", (&trans)))
+      if (!SyncAssert(!kernel_->parent_child_index.Contains(entry.get()),
+                      FROM_HERE, "Deleted entry still present", (&trans)))
         return false;
       RemoveFromAttachmentIndex(lock, entry->ref(META_HANDLE),
                                 entry->ref(ATTACHMENT_METADATA));
@@ -710,17 +710,18 @@ void Directory::UnapplyEntry(EntryKernel* entry) {
 
 void Directory::DeleteEntry(const ScopedKernelLock& lock,
                             bool save_to_journal,
-                            EntryKernel* entry,
+                            EntryKernel* entry_ptr,
                             OwnedEntryKernelSet* entries_to_journal) {
-  int64_t handle = entry->ref(META_HANDLE);
-  ModelType server_type =
-      GetModelTypeFromSpecifics(entry->ref(SERVER_SPECIFICS));
+  int64_t handle = entry_ptr->ref(META_HANDLE);
 
   kernel_->metahandles_to_purge.insert(handle);
 
-  std::unique_ptr<EntryKernel> entry_ptr =
+  std::unique_ptr<EntryKernel> entry =
       std::move(kernel_->metahandles_map[handle]);
 
+  ModelType server_type =
+      GetModelTypeFromSpecifics(entry->ref(SERVER_SPECIFICS));
+
   size_t num_erased = 0;
   num_erased = kernel_->metahandles_map.erase(handle);
   DCHECK_EQ(1u, num_erased);
@@ -730,8 +731,8 @@ void Directory::DeleteEntry(const ScopedKernelLock& lock,
   DCHECK_EQ(entry->ref(IS_UNSYNCED), num_erased > 0);
   num_erased = kernel_->unapplied_update_metahandles[server_type].erase(handle);
   DCHECK_EQ(entry->ref(IS_UNAPPLIED_UPDATE), num_erased > 0);
-  if (kernel_->parent_child_index.Contains(entry))
-    kernel_->parent_child_index.Remove(entry);
+  if (kernel_->parent_child_index.Contains(entry.get()))
+    kernel_->parent_child_index.Remove(entry.get());
 
   if (!entry->ref(UNIQUE_CLIENT_TAG).empty()) {
     num_erased = kernel_->client_tags_map.erase(entry->ref(UNIQUE_CLIENT_TAG));
@@ -744,7 +745,7 @@ void Directory::DeleteEntry(const ScopedKernelLock& lock,
   RemoveFromAttachmentIndex(lock, handle, entry->ref(ATTACHMENT_METADATA));
 
   if (save_to_journal) {
-    entries_to_journal->insert(std::move(entry_ptr));
+    entries_to_journal->insert(std::move(entry));
   }
 }
 

components/sync/syncable/directory.h

@@ -500,7 +500,8 @@ class Directory {
                                 AttachmentIdList* ids);
 
   // For new entry creation only.
-  bool InsertEntry(BaseWriteTransaction* trans, EntryKernel* entry);
+  bool InsertEntry(BaseWriteTransaction* trans,
+                   std::unique_ptr<EntryKernel> entry);
 
   // Update the attachment index for |metahandle| removing it from the index
   // under |old_metadata| entries and add it under |new_metadata| entries.
@@ -557,7 +558,7 @@ class Directory {
 
   bool InsertEntry(const ScopedKernelLock& lock,
                    BaseWriteTransaction* trans,
-                   EntryKernel* entry);
+                   std::unique_ptr<EntryKernel> entry);
 
   // Remove each of |metahandle|'s attachment ids from index_by_attachment_id.
   void RemoveFromAttachmentIndex(

components/sync/syncable/model_neutral_mutable_entry.cc

@@ -5,6 +5,7 @@
 #include "components/sync/syncable/model_neutral_mutable_entry.h"
 
 #include <memory>
+#include <utility>
 
 #include "components/sync/base/hash_util.h"
 #include "components/sync/base/unique_position.h"
@@ -37,12 +38,12 @@ ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,
   kernel->put(IS_DEL, true);
   // We match the database defaults here
   kernel->put(BASE_VERSION, CHANGES_VERSION);
-  if (!trans->directory()->InsertEntry(trans, kernel.get())) {
+  kernel_ = kernel.get();
+  if (!trans->directory()->InsertEntry(trans, std::move(kernel))) {
+    kernel_ = nullptr;
     return;  // Failed inserting.
   }
-  trans->TrackChangesTo(kernel.get());
-
-  kernel_ = kernel.release();
+  trans->TrackChangesTo(kernel_);
 }
 
 ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,
@@ -74,14 +75,13 @@ ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,
   kernel->put(IS_DIR, true);
 
   kernel->mark_dirty(&trans->directory()->kernel()->dirty_metahandles);
+  kernel_ = kernel.get();
 
-  if (!trans->directory()->InsertEntry(trans, kernel.get())) {
+  if (!trans->directory()->InsertEntry(trans, std::move(kernel))) {
+    kernel_ = nullptr;
     return;  // Failed inserting.
   }
-
-  trans->TrackChangesTo(kernel.get());
-
-  kernel_ = kernel.release();
+  trans->TrackChangesTo(kernel_);
 }
 
 ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,

components/sync/syncable/mutable_entry.cc

@@ -4,7 +4,7 @@
 
 #include "components/sync/syncable/mutable_entry.h"
 
-#include <memory>
+#include <utility>
 
 #include "components/sync/base/hash_util.h"
 #include "components/sync/base/unique_position.h"
@@ -19,55 +19,19 @@ using std::string;
 namespace syncer {
 namespace syncable {
 
-void MutableEntry::Init(WriteTransaction* trans,
-                        ModelType model_type,
-                        const Id& parent_id,
-                        const string& name) {
-  std::unique_ptr<EntryKernel> kernel(new EntryKernel);
-  kernel_ = nullptr;
-
-  kernel->put(ID, trans->directory_->NextId());
-  kernel->put(META_HANDLE, trans->directory_->NextMetahandle());
-  kernel->mark_dirty(&trans->directory_->kernel()->dirty_metahandles);
-  kernel->put(NON_UNIQUE_NAME, name);
-  const base::Time& now = base::Time::Now();
-  kernel->put(CTIME, now);
-  kernel->put(MTIME, now);
-  // We match the database defaults here
-  kernel->put(BASE_VERSION, CHANGES_VERSION);
-
-  if (!parent_id.IsNull()) {
-    kernel->put(PARENT_ID, parent_id);
-  }
-
-  // Normally the SPECIFICS setting code is wrapped in logic to deal with
-  // unknown fields and encryption.  Since all we want to do here is ensure that
-  // GetModelType() returns a correct value from the very beginning, these
-  // few lines are sufficient.
-  sync_pb::EntitySpecifics specifics;
-  AddDefaultFieldValue(model_type, &specifics);
-  kernel->put(SPECIFICS, specifics);
-
-  // Because this entry is new, it was originally deleted.
-  kernel->put(IS_DEL, true);
-  trans->TrackChangesTo(kernel.get());
-  kernel->put(IS_DEL, false);
-
-  // Now swap the pointers.
-  kernel_ = kernel.release();
-}
-
 MutableEntry::MutableEntry(WriteTransaction* trans,
                            Create,
                            ModelType model_type,
                            const string& name)
     : ModelNeutralMutableEntry(trans), write_transaction_(trans) {
-  Init(trans, model_type, Id(), name);
+  std::unique_ptr<EntryKernel> kernel =
+      CreateEntryKernel(trans, model_type, Id(), name);
+  kernel_ = kernel.get();
   // We need to have a valid position ready before we can index the item.
   DCHECK_NE(BOOKMARKS, model_type);
   DCHECK(!ShouldMaintainPosition());
 
-  bool result = trans->directory()->InsertEntry(trans, kernel_);
+  bool result = trans->directory()->InsertEntry(trans, std::move(kernel));
   DCHECK(result);
 }
 
@@ -77,7 +41,9 @@ MutableEntry::MutableEntry(WriteTransaction* trans,
                            const Id& parent_id,
                            const string& name)
     : ModelNeutralMutableEntry(trans), write_transaction_(trans) {
-  Init(trans, model_type, parent_id, name);
+  std::unique_ptr<EntryKernel> kernel =
+      CreateEntryKernel(trans, model_type, parent_id, name);
+  kernel_ = kernel.get();
   // We need to have a valid position ready before we can index the item.
   if (model_type == BOOKMARKS) {
     // Base the tag off of our cache-guid and local "c-" style ID.
@@ -89,7 +55,7 @@ MutableEntry::MutableEntry(WriteTransaction* trans,
     DCHECK(!ShouldMaintainPosition());
   }
 
-  bool result = trans->directory()->InsertEntry(trans, kernel_);
+  bool result = trans->directory()->InsertEntry(trans, std::move(kernel));
   DCHECK(result);
 }
 
@@ -303,6 +269,43 @@ void MutableEntry::MarkAttachmentAsOnServer(
   MarkForSyncing(this);
 }
 
+// static
+std::unique_ptr<EntryKernel> MutableEntry::CreateEntryKernel(
+    WriteTransaction* trans,
+    ModelType model_type,
+    const Id& parent_id,
+    const string& name) {
+  std::unique_ptr<EntryKernel> kernel(new EntryKernel);
+
+  kernel->put(ID, trans->directory_->NextId());
+  kernel->put(META_HANDLE, trans->directory_->NextMetahandle());
+  kernel->mark_dirty(&trans->directory_->kernel()->dirty_metahandles);
+  kernel->put(NON_UNIQUE_NAME, name);
+  const base::Time& now = base::Time::Now();
+  kernel->put(CTIME, now);
+  kernel->put(MTIME, now);
+  // We match the database defaults here
+  kernel->put(BASE_VERSION, CHANGES_VERSION);
+
+  if (!parent_id.IsNull()) {
+    kernel->put(PARENT_ID, parent_id);
+  }
+
+  // Normally the SPECIFICS setting code is wrapped in logic to deal with
+  // unknown fields and encryption.  Since all we want to do here is ensure that
+  // GetModelType() returns a correct value from the very beginning, these
+  // few lines are sufficient.
+  sync_pb::EntitySpecifics specifics;
+  AddDefaultFieldValue(model_type, &specifics);
+  kernel->put(SPECIFICS, specifics);
+
+  // Because this entry is new, it was originally deleted.
+  kernel->put(IS_DEL, true);
+  trans->TrackChangesTo(kernel.get());
+  kernel->put(IS_DEL, false);
+  return kernel;
+}
+
 // This function sets only the flags needed to get this entry to sync.
 bool MarkForSyncing(MutableEntry* e) {
   DCHECK_NE(static_cast<MutableEntry*>(nullptr), e);

components/sync/syncable/mutable_entry.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 
 #include "base/macros.h"
@@ -26,11 +27,6 @@ class WriteTransaction;
 // A mutable meta entry.  Changes get committed to the database when the
 // WriteTransaction is destroyed.
 class MutableEntry : public ModelNeutralMutableEntry {
-  void Init(WriteTransaction* trans,
-            ModelType model_type,
-            const Id& parent_id,
-            const std::string& name);
-
  public:
   MutableEntry(WriteTransaction* trans, CreateNewUpdateItem, const Id& id);
   MutableEntry(WriteTransaction* trans,
@@ -77,6 +73,12 @@ class MutableEntry : public ModelNeutralMutableEntry {
       const sync_pb::AttachmentIdProto& attachment_id);
 
  private:
+  static std::unique_ptr<EntryKernel> CreateEntryKernel(
+      WriteTransaction* trans,
+      ModelType model_type,
+      const Id& parent_id,
+      const std::string& name);
+
   // Kind of redundant. We should reduce the number of pointers
   // floating around if at all possible. Could we store this in Directory?
   // Scope: Set on construction, never changed after that.