Bug 1488453 - Make sure there are no UAF or double frees in HashTable code, r=froydnj.
authorBrian Hackett <bhackett1024@gmail.com>
Mon, 29 Oct 2018 12:35:20 -1000
changeset 443422 3f6e29ae6529c657e027b8d1fd36a7daaf501526
parent 443421 8ff29f3d64b79cfe630e5ce302fc2bca7e02f425
child 443423 b7ed6370bc7d11c486ca86f74ba7e4bc00b471af
push id109365
push userbhackett@mozilla.com
push dateMon, 29 Oct 2018 22:36:35 +0000
treeherdermozilla-inbound@3f6e29ae6529 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1488453
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1488453 - Make sure there are no UAF or double frees in HashTable code, r=froydnj.
toolkit/recordreplay/HashTable.cpp
--- a/toolkit/recordreplay/HashTable.cpp
+++ b/toolkit/recordreplay/HashTable.cpp
@@ -11,16 +11,18 @@
 #include "HashTable.h"
 #include "InfallibleVector.h"
 #include "ProcessRecordReplay.h"
 #include "ProcessRedirect.h"
 #include "ValueIndex.h"
 
 #include "PLDHashTable.h"
 
+#include <unordered_set>
+
 namespace mozilla {
 namespace recordreplay {
 
 // Hash tables frequently incorporate pointer values into the hash numbers they
 // compute, which are not guaranteed to be the same between recording and
 // replaying and consequently lead to inconsistent hash numbers and iteration
 // order between recording and replaying, which can in turn affect the order in
 // which recorded events occur. HashTable stabilization is designed to deal
@@ -39,24 +41,16 @@ namespace recordreplay {
 // hash of an arbitrary key, we look for a matching key in the table, using
 // that key's hash if found. Otherwise, a new hash is generated from an
 // incrementing counter.
 
 typedef uint32_t HashNumber;
 
 class StableHashTableInfo
 {
-  // Magic number for attempting to determine whether we are dealing with an
-  // actual StableHashTableInfo. Despite our best efforts some hashtables do
-  // not go through stabilization (e.g. they have static constructors that run
-  // before record/replay state is initialized).
-  size_t mMagic;
-
-  static const size_t MagicNumber = 0xDEADBEEFDEADBEEF;
-
   // Information about a key in the table: the key pointer, along with the new
   // hash number we have generated for the key.
   struct KeyInfo {
     const void* mKey;
     HashNumber mNewHash;
   };
 
   // Table mapping original hash numbers (produced by the table's hash
@@ -79,16 +73,20 @@ class StableHashTableInfo
   // Counter for generating new hash numbers for entries added to the table.
   // This increases monotonically, though it is fine if it overflows.
   uint32_t mHashGenerator;
 
   // Buffer with executable memory for use in binding functions.
   uint8_t* mCallbackStorage;
   static const size_t CallbackStorageCapacity = 4096;
 
+  // Whether this table has been marked as destroyed and is unusable. This is
+  // temporary state to detect UAF bugs related to this class.
+  bool mDestroyed;
+
   // Get an existing key in the table.
   KeyInfo* FindKeyInfo(HashNumber aOriginalHash, const void* aKey, HashInfo** aHashInfo = nullptr) {
     HashToKeyMap::iterator iter = mHashToKey.find(aOriginalHash);
     MOZ_RELEASE_ASSERT(iter != mHashToKey.end());
 
     HashInfo* hashInfo = iter->second.get();
     for (KeyInfo& keyInfo : hashInfo->mKeys) {
       if (keyInfo.mKey == aKey) {
@@ -98,33 +96,42 @@ class StableHashTableInfo
         return &keyInfo;
       }
     }
     MOZ_CRASH();
   }
 
 public:
   StableHashTableInfo()
-    : mMagic(MagicNumber)
-    , mLastKey(nullptr)
+    : mLastKey(nullptr)
     , mLastNewHash(0)
     , mHashGenerator(0)
     , mCallbackStorage(nullptr)
+    , mDestroyed(false)
   {
     // Use AllocateMemory, as the result will have RWX permissions.
     mCallbackStorage = (uint8_t*) AllocateMemory(CallbackStorageCapacity, MemoryKind::Tracked);
+
+    MarkValid();
   }
 
   ~StableHashTableInfo() {
     MOZ_RELEASE_ASSERT(mHashToKey.empty());
     DeallocateMemory(mCallbackStorage, CallbackStorageCapacity, MemoryKind::Tracked);
+
+    UnmarkValid();
   }
 
-  bool AppearsValid() {
-    return mMagic == MagicNumber;
+  bool IsDestroyed() {
+    return mDestroyed;
+  }
+
+  void MarkDestroyed() {
+    MOZ_RELEASE_ASSERT(!IsDestroyed());
+    mDestroyed = true;
   }
 
   void AddKey(HashNumber aOriginalHash, const void* aKey, HashNumber aNewHash) {
     HashToKeyMap::iterator iter = mHashToKey.find(aOriginalHash);
     if (iter == mHashToKey.end()) {
       iter = mHashToKey.insert(HashToKeyMap::value_type(aOriginalHash, MakeUnique<HashInfo>())).first;
     }
     HashInfo* hashInfo = iter->second.get();
@@ -227,18 +234,46 @@ public:
 
     aOther.mHashToKey.clear();
     aOther.mKeyToHash.clear();
     aOther.mHashGenerator = 0;
 
     mLastKey = aOther.mLastKey = nullptr;
     mLastNewHash = aOther.mLastNewHash = 0;
   }
+
+  // Set of valid StableHashTableInfo pointers. We use this to determine whether
+  // we are dealing with an actual StableHashTableInfo. Despite our best efforts
+  // some hashtables do not go through stabilization (e.g. they have static
+  // constructors that run before record/replay state is initialized).
+  static std::unordered_set<StableHashTableInfo*>* gHashInfos;
+  static SpinLock gHashInfosLock;
+
+  inline bool IsValid() {
+    AutoSpinLock lock(gHashInfosLock);
+    return gHashInfos && gHashInfos->find(this) != gHashInfos->end();
+  }
+
+  inline void MarkValid() {
+    AutoSpinLock lock(gHashInfosLock);
+    if (!gHashInfos) {
+      gHashInfos = new std::unordered_set<StableHashTableInfo*>();
+    }
+    gHashInfos->insert(this);
+  }
+
+  inline void UnmarkValid() {
+    AutoSpinLock lock(gHashInfosLock);
+    gHashInfos->erase(this);
+  }
 };
 
+std::unordered_set<StableHashTableInfo*>* StableHashTableInfo::gHashInfos;
+SpinLock StableHashTableInfo::gHashInfosLock;
+
 ///////////////////////////////////////////////////////////////////////////////
 // PLHashTable Stabilization
 ///////////////////////////////////////////////////////////////////////////////
 
 // For each PLHashTable in the process, a PLHashTableInfo is generated. This
 // structure becomes the |allocPriv| for the table, handled by the new
 // callbacks given to it.
 struct PLHashTableInfo : public StableHashTableInfo
@@ -259,17 +294,17 @@ struct PLHashTableInfo : public StableHa
       mKeyCompare(aKeyCompare),
       mValueCompare(aValueCompare),
       mAllocOps(aAllocOps),
       mAllocPrivate(aAllocPrivate)
   {}
 
   static PLHashTableInfo* FromPrivate(void* aAllocPrivate) {
     PLHashTableInfo* info = reinterpret_cast<PLHashTableInfo*>(aAllocPrivate);
-    MOZ_RELEASE_ASSERT(info->AppearsValid());
+    MOZ_RELEASE_ASSERT(!info->IsDestroyed());
     return info;
   }
 };
 
 static void*
 WrapPLHashAllocTable(void* aAllocPrivate, PRSize aSize)
 {
   PLHashTableInfo* info = PLHashTableInfo::FromPrivate(aAllocPrivate);
@@ -331,16 +366,17 @@ WrapPLHashFreeEntry(void *aAllocPrivate,
 static PLHashAllocOps gWrapPLHashAllocOps = {
   WrapPLHashAllocTable, WrapPLHashFreeTable,
   WrapPLHashAllocEntry, WrapPLHashFreeEntry
 };
 
 static uint32_t
 PLHashComputeHash(void* aKey, PLHashTableInfo* aInfo)
 {
+  MOZ_RELEASE_ASSERT(!aInfo->IsDestroyed());
   uint32_t originalHash = aInfo->mKeyHash(aKey);
   HashNumber newHash;
   if (aInfo->HasMatchingKey(originalHash,
                             [=](const void* aExistingKey) {
                               return aInfo->mKeyCompare(aKey, aExistingKey);
                             }, &newHash)) {
     return newHash;
   }
@@ -361,17 +397,18 @@ GeneratePLHashTableCallbacks(PLHashFunct
   *aAllocOps = &gWrapPLHashAllocOps;
   *aAllocPrivate = info;
 }
 
 void
 DestroyPLHashTableCallbacks(void* aAllocPrivate)
 {
   PLHashTableInfo* info = PLHashTableInfo::FromPrivate(aAllocPrivate);
-  delete info;
+  info->MarkDestroyed();
+  //delete info;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // PLDHashTable Stabilization
 ///////////////////////////////////////////////////////////////////////////////
 
 // For each PLDHashTable in the process, a PLDHashTableInfo is generated. This
 // structure is supplied to its callbacks using bound functions.
@@ -387,65 +424,74 @@ struct PLDHashTableInfo : public StableH
     : mOps(aOps)
   {
     PodZero(&mNewOps);
   }
 
   static PLDHashTableInfo* MaybeFromOps(const PLDHashTableOps* aOps) {
     PLDHashTableInfo* res = reinterpret_cast<PLDHashTableInfo*>
       ((uint8_t*)aOps - offsetof(PLDHashTableInfo, mNewOps));
-    return res->AppearsValid() ? res : nullptr;
+    if (res->IsValid()) {
+      MOZ_RELEASE_ASSERT(!res->IsDestroyed());
+      return res;
+    }
+    return nullptr;
   }
 
   static PLDHashTableInfo* FromOps(const PLDHashTableOps* aOps) {
     PLDHashTableInfo* res = MaybeFromOps(aOps);
     MOZ_RELEASE_ASSERT(res);
     return res;
   }
 };
 
 static PLDHashNumber
 PLDHashTableComputeHash(const void* aKey, PLDHashTableInfo* aInfo)
 {
+  MOZ_RELEASE_ASSERT(!aInfo->IsDestroyed());
   uint32_t originalHash = aInfo->mOps->hashKey(aKey);
   HashNumber newHash;
   if (aInfo->HasMatchingKey(originalHash,
                             [=](const void* aExistingKey) {
                               return aInfo->mOps->matchEntry((PLDHashEntryHdr*) aExistingKey, aKey);
                             }, &newHash)) {
     return newHash;
   }
   return aInfo->SetLastKey(aKey);
 }
 
 static void
 PLDHashTableMoveEntry(PLDHashTable* aTable, const PLDHashEntryHdr* aFrom, PLDHashEntryHdr* aTo,
                       PLDHashTableInfo* aInfo)
 {
+  MOZ_RELEASE_ASSERT(!aInfo->IsDestroyed());
   aInfo->mOps->moveEntry(aTable, aFrom, aTo);
 
   uint32_t originalHash = aInfo->GetOriginalHashNumber(aFrom);
   uint32_t newHash = aInfo->FindKeyHash(originalHash, aFrom);
 
   aInfo->RemoveKey(originalHash, aFrom);
   aInfo->AddKey(originalHash, aTo, newHash);
 }
 
 static void
 PLDHashTableClearEntry(PLDHashTable* aTable, PLDHashEntryHdr* aEntry, PLDHashTableInfo* aInfo)
 {
+  MOZ_RELEASE_ASSERT(!aInfo->IsDestroyed());
   aInfo->mOps->clearEntry(aTable, aEntry);
 
   uint32_t originalHash = aInfo->GetOriginalHashNumber(aEntry);
   aInfo->RemoveKey(originalHash, aEntry);
 }
 
 static void
 PLDHashTableInitEntry(PLDHashEntryHdr* aEntry, const void* aKey, PLDHashTableInfo* aInfo)
 {
+  MOZ_RELEASE_ASSERT(!aInfo->IsDestroyed());
+
   if (aInfo->mOps->initEntry) {
     aInfo->mOps->initEntry(aEntry, aKey);
   }
 
   uint32_t originalHash = aInfo->mOps->hashKey(aKey);
   aInfo->AddKey(originalHash, aEntry, aInfo->GetLastNewHash(aKey));
 }
 
@@ -476,19 +522,22 @@ RecordReplayInterface_InternalDestroyPLD
 {
   // Primordial PLDHashTables used in the copy constructor might not have any ops.
   if (!aOps) {
     return;
   }
 
   // Note: PLDHashTables with static ctors might have been constructed before
   // record/replay state was initialized and have their normal ops. Check the
-  // magic number via MaybeFromOps before destroying the info.
+  // info is valid via MaybeFromOps before destroying the info.
   PLDHashTableInfo* info = PLDHashTableInfo::MaybeFromOps(aOps);
-  delete info;
+  if (info) {
+    info->MarkDestroyed();
+  }
+  //delete info;
 }
 
 MOZ_EXPORT void
 RecordReplayInterface_InternalMovePLDHashTableContents(const PLDHashTableOps* aFirstOps,
                                                        const PLDHashTableOps* aSecondOps)
 {
   PLDHashTableInfo* firstInfo = PLDHashTableInfo::FromOps(aFirstOps);
   PLDHashTableInfo* secondInfo = PLDHashTableInfo::FromOps(aSecondOps);