Bug 845613 - Locking issues in the write poisoning code. r=BenWa.

--HG--
extra : rebase_source : 49cbd361b1cd22e7d47546ec1c0292674b156f07

xpcom/build/mozPoisonWriteBase.cpp

@@ -32,9 +32,16 @@ using namespace mozilla;
 
 namespace mozilla {
 
+PRLock *DebugFDAutoLock::Lock;
+void DebugFDAutoLock::Clear() {
+  MOZ_ASSERT(Lock != nullptr);
+  Lock = nullptr;
+}
+
 static char *sProfileDirectory = NULL;
 
 std::vector<int>& getDebugFDs() {
+  PR_ASSERT_CURRENT_THREAD_OWNS_LOCK(DebugFDAutoLock::getDebugFDsLock());
   // We have to use new as some write happen during static destructors
   // so an static std::vector might be destroyed while we still need it.
   static std::vector<int> *DebugFDs = new std::vector<int>();
@@ -43,6 +50,9 @@ std::vector<int>& getDebugFDs() {
 
 void InitWritePoisoning()
 {
+  // Call to make sure it is initialized.
+  DebugFDAutoLock::getDebugFDsLock();
+
   nsCOMPtr<nsIFile> mozFile;
   NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mozFile));
   if (mozFile) {
@@ -54,13 +64,6 @@ void InitWritePoisoning()
   }
 }
 
-void BaseCleanup() {
-  PL_strfree(sProfileDirectory);
-  sProfileDirectory = nullptr;
-  delete &getDebugFDs();
-  PR_DestroyLock(DebugFDAutoLock::getDebugFDsLock());
-}
-
 // This a wrapper over a file descriptor that provides a Printf method and
 // computes the sha1 of the data that passes through it.
 class SHA1Stream
@@ -219,10 +222,21 @@ enum PoisonState {
 PoisonState sPoisoningState = POISON_UNINITIALIZED;
 
 void DisableWritePoisoning() {
-  if (sPoisoningState == POISON_ON) {
+  if (sPoisoningState != POISON_ON)
+    return;
+
   sPoisoningState = POISON_OFF;
-    BaseCleanup();
+  PL_strfree(sProfileDirectory);
+  sProfileDirectory = nullptr;
+
+  PRLock *Lock;
+  {
+    DebugFDAutoLock lockedScope;
+    delete &getDebugFDs();
+    Lock = DebugFDAutoLock::getDebugFDsLock();
+    DebugFDAutoLock::Clear();
   }
+  PR_DestroyLock(Lock);
 }
 void EnableWritePoisoning() {
   sPoisoningState = POISON_ON;

xpcom/build/mozPoisonWriteBase.h

@@ -30,11 +30,24 @@ struct DebugFDAutoLockTraits {
 };
 
 class DebugFDAutoLock : public Scoped<DebugFDAutoLockTraits> {
+  static PRLock *Lock;
 public:
+  static void Clear();
   static PRLock *getDebugFDsLock() {
+    // On windows this static is not thread safe, but we know that the first
+    // call is from
+    // * An early registration of a debug FD or
+    // * The call to InitWritePoisoning.
+    // Since the early debug FDs are logs created early in the main thread
+    // and no writes are trapped before InitWritePoisoning, we are safe.
+    static bool Initialized = false;
+    if (!Initialized) {
+      Lock = PR_NewLock();
+      Initialized = true;
+    }
+
     // We have to use something lower level than a mutex. If we don't, we
     // can get recursive in here when called from logging a call to free.
-    static PRLock *Lock = PR_NewLock();
     return Lock;
   }
 

xpcom/build/mozPoisonWriteMac.cpp

@@ -135,30 +135,6 @@ FuncData *Functions[] = { &aio_write_data,
 
 const int NumFunctions = ArrayLength(Functions);
 
-struct AutoLockTraits {
-    typedef PRLock *type;
-    const static type empty() {
-      return NULL;
-    }
-    const static void release(type aL) {
-        PR_Unlock(aL);
-    }
-};
-
-class MyAutoLock : public Scoped<AutoLockTraits> {
-public:
-    static PRLock *getDebugFDsLock() {
-        // We have to use something lower level than a mutex. If we don't, we
-        // can get recursive in here when called from logging a call to free.
-        static PRLock *Lock = PR_NewLock();
-        return Lock;
-    }
-
-    MyAutoLock() : Scoped<AutoLockTraits>(getDebugFDsLock()) {
-        PR_Lock(get());
-    }
-};
-
 // We want to detect "actual" writes, not IPC. Some IPC mechanisms are
 // implemented with file descriptors, so filter them out.
 bool IsIPCWrite(int fd, const struct stat &buf) {
@@ -200,7 +176,7 @@ void AbortOnBadWrite(int fd, const void *wbuf, size_t count) {
         return;
 
     {
-        MyAutoLock lockedScope;
+        DebugFDAutoLock lockedScope;
 
         // Debugging FDs are OK
         std::vector<int> &Vec = getDebugFDs();