Bug 1451891 - Fix race conditions in __wrap_dlerror; r?glandium draft
authorJim Chen <nchen@mozilla.com>
Wed, 25 Jul 2018 13:59:30 -0400
changeset 822682 a4bbbad45aeeb4279d1000ddfd16fab3781e809a
parent 822681 6131c47a97a8b0912f1ab0e02650b2de0a89d67a
child 823238 65034a13d6878f4da25bb67e9369a2ee8648991f
push id117443
push userbmo:nchen@mozilla.com
push dateWed, 25 Jul 2018 18:01:42 +0000
reviewersglandium
bugs1451891
milestone63.0a1
Bug 1451891 - Fix race conditions in __wrap_dlerror; r?glandium __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();