Bug 1451891 - Fix race conditions in __wrap_dlerror. r=glandium a=pascalc
authorJim Chen <nchen@mozilla.com>
Wed, 25 Jul 2018 13:59:30 -0400
changeset 480675 c43d2dbaf59e73747f25a2d8c5bcd68c28e5cadf
parent 480674 217cbc03f5dd5dab710fcc346596588677c987d6
child 480676 4a084c06e9964ce8587128281e0aa8a80fe57738
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium, pascalc
bugs1451891
milestone62.0
Bug 1451891 - Fix race conditions in __wrap_dlerror. r=glandium a=pascalc __wrap_dlerror uses a single pointer for all threads, which means one thread could get the dlerror result from another thread. Normally this wouldn't cause crashes. However, because dlerror results come from a per-thread buffer, if a thread exits and our saved dlerror result came from that thread, the saved pointer could then refer to invalid memory. The proper way to fix this is to use TLS and have a per-thread pointer for __wrap_dlerror. However, instead of using up a TLS slot, this patch keeps the single pointer for custom messages, and fallback to per-thread dlerror call for system messages. While the race condition still exists, I think the risk is acceptable. Even when races occur, they should no longer cause crashes. MozReview-Commit-ID: 4hGksidjiVz
mozglue/linker/ElfLoader.cpp
mozglue/linker/ElfLoader.h
--- a/mozglue/linker/ElfLoader.cpp
+++ b/mozglue/linker/ElfLoader.cpp
@@ -70,37 +70,39 @@ void *
   if (handle)
     handle->AddDirectRef();
   return handle;
 }
 
 const char *
 __wrap_dlerror(void)
 {
-  const char *error = ElfLoader::Singleton.lastError;
-  ElfLoader::Singleton.lastError = nullptr;
-  return error;
+  const char* error = ElfLoader::Singleton.lastError.exchange(nullptr);
+  if (error) {
+      // Return a custom error if available.
+      return error;
+  }
+  // Or fallback to the system error.
+  return dlerror();
 }
 
 void *
 __wrap_dlsym(void *handle, const char *symbol)
 {
   if (!handle) {
     ElfLoader::Singleton.lastError = "dlsym(NULL, sym) unsupported";
     return nullptr;
   }
   if (handle != RTLD_DEFAULT && handle != RTLD_NEXT) {
     LibHandle *h = reinterpret_cast<LibHandle *>(handle);
     return h->GetSymbolPtr(symbol);
   }
 
-  void* sym = dlsym(handle, symbol);
-  ElfLoader::Singleton.lastError = dlerror();
-
-  return sym;
+  ElfLoader::Singleton.lastError = nullptr; // Use system dlerror.
+  return dlsym(handle, symbol);
 }
 
 int
 __wrap_dlclose(void *handle)
 {
   if (!handle) {
     ElfLoader::Singleton.lastError = "No handle given to dlclose()";
     return -1;
@@ -415,44 +417,44 @@ SystemElf::Load(const char *path, int fl
   /* The Android linker returns a handle when the file name matches an
    * already loaded library, even when the full path doesn't exist */
   if (path && path[0] == '/' && (access(path, F_OK) == -1)){
     DEBUG_LOG("dlopen(\"%s\", 0x%x) = %p", path, flags, (void *)nullptr);
     ElfLoader::Singleton.lastError = "Specified file does not exist";
     return nullptr;
   }
 
+  ElfLoader::Singleton.lastError = nullptr; // Use system dlerror.
   void *handle = dlopen(path, flags);
   DEBUG_LOG("dlopen(\"%s\", 0x%x) = %p", path, flags, handle);
-  ElfLoader::Singleton.lastError = dlerror();
   if (handle) {
     SystemElf *elf = new SystemElf(path, handle);
     ElfLoader::Singleton.Register(elf);
     RefPtr<LibHandle> lib(elf);
     return lib.forget();
   }
   return nullptr;
 }
 
 SystemElf::~SystemElf()
 {
   if (!dlhandle)
     return;
   DEBUG_LOG("dlclose(%p [\"%s\"])", dlhandle, GetPath());
+  ElfLoader::Singleton.lastError = nullptr; // Use system dlerror.
   dlclose(dlhandle);
-  ElfLoader::Singleton.lastError = dlerror();
   ElfLoader::Singleton.Forget(this);
 }
 
 void *
 SystemElf::GetSymbolPtr(const char *symbol) const
 {
+  ElfLoader::Singleton.lastError = nullptr; // Use system dlerror.
   void *sym = dlsym(dlhandle, symbol);
   DEBUG_LOG("dlsym(%p [\"%s\"], \"%s\") = %p", dlhandle, GetPath(), symbol, sym);
-  ElfLoader::Singleton.lastError = dlerror();
   return sym;
 }
 
 Mappable *
 SystemElf::GetMappable() const
 {
   const char *path = GetPath();
   if (!path)
--- a/mozglue/linker/ElfLoader.h
+++ b/mozglue/linker/ElfLoader.h
@@ -3,16 +3,17 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef ElfLoader_h
 #define ElfLoader_h
 
 #include <vector>
 #include <dlfcn.h>
 #include <signal.h>
+#include "mozilla/Atomics.h"
 #include "mozilla/RefCounted.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/UniquePtr.h"
 #include "Zip.h"
 #include "Elfxx.h"
 #include "Mappable.h"
 
 /**
@@ -441,22 +442,23 @@ protected:
 
   /**
    * Forget about the given handle. This method is meant to be called by
    * LibHandle subclass destructors.
    */
   void Forget(LibHandle *handle);
   void Forget(CustomElf *handle);
 
-  /* Last error. Used for dlerror() */
   friend class SystemElf;
   friend const char *__wrap_dlerror(void);
   friend void *__wrap_dlsym(void *handle, const char *symbol);
   friend int __wrap_dlclose(void *handle);
-  const char *lastError;
+  /* __wrap_dlerror() returns this custom last error if non-null or the system
+   * dlerror() value if this is null. Must refer to a string constant. */
+  mozilla::Atomic<const char*, mozilla::Relaxed> lastError;
 
 private:
   ElfLoader() : expect_shutdown(true), lastError(nullptr)
   {
     pthread_mutex_init(&handlesMutex, nullptr);
   }
 
   ~ElfLoader();