Bug 1483865: Make NotStackAllocated assertion a bit safer. r=froydnj
authorKris Maglione <maglione.k@gmail.com>
Thu, 16 Aug 2018 11:35:50 -0700
changeset 481196 47149b145d5799b924aa9416c761aab210b4507c
parent 481195 db6e8f0ee6b45d11799fbf9fa2924d49b7c14480
child 481197 366a2aa802b5a7bd06328a7162f10292cbde3411
push id232
push userfmarier@mozilla.com
push dateWed, 05 Sep 2018 20:45:54 +0000
reviewersfroydnj
bugs1483865
milestone63.0a1
Bug 1483865: Make NotStackAllocated assertion a bit safer. r=froydnj This assertion was always meant to be a best-effort thing to catch obvious errors, but the cases where the assumptions it makes fail have been growing. I could remove it entirely, but I'd be happier keeping at least some basic sanity checks. This compromise continues allowing any address below the first argument pointer, and extends the assertion to also allow anything more than 2KiB above it. We could probably get away with stretching that to at least 4KiB, but 2 seems safer, and likely enough to catch the obvious cases. Differential Revision: https://phabricator.services.mozilla.com/D3542
xpcom/components/nsComponentManager.cpp
--- a/xpcom/components/nsComponentManager.cpp
+++ b/xpcom/components/nsComponentManager.cpp
@@ -475,23 +475,42 @@ AssertNotMallocAllocated(T* aPtr)
   MOZ_ASSERT(info.tag == TagUnknown);
 #endif
 }
 
 template<typename T>
 static void
 AssertNotStackAllocated(T* aPtr)
 {
-  // The main thread's stack should be allocated at the top of our address
-  // space. Anything stack allocated should be above us on the stack, and
-  // therefore above our first argument pointer.
-  // Only this is apparently not the case on Windows.
-#if !(defined(XP_WIN) || defined(ANDROID))
-  MOZ_ASSERT(NS_IsMainThread());
-  MOZ_ASSERT(uintptr_t(aPtr) < uintptr_t(&aPtr));
+  // On all of our supported platforms, the stack grows down. Any address
+  // located below the address of our argument is therefore guaranteed not to be
+  // stack-allocated by the caller.
+  //
+  // For addresses above our argument, things get trickier. The main thread
+  // stack is traditionally placed at the top of the program's address space,
+  // but that is becoming less reliable as more and more systems adopt address
+  // space layout randomization strategies, so we have to guess how much space
+  // above our argument pointer we need to care about.
+  //
+  // On most systems, we're guaranteed at least several KiB at the top of each
+  // stack for TLS. We'd probably be safe assuming at least 4KiB in the stack
+  // segment above our argument address, but safer is... well, safer.
+  //
+  // For threads with huge stacks, it's theoretically possible that we could
+  // wind up being passed a stack-allocated string from farther up the stack,
+  // but this is a best-effort thing, so we'll assume we only care about the
+  // immediate caller. For that case, max 2KiB per stack frame is probably a
+  // reasonable guess most of the time, and is less than the ~4KiB that we
+  // expect for TLS, so go with that to avoid the risk of bumping into heap
+  // data just above the stack.
+#ifdef DEBUG
+  static constexpr size_t kFuzz = 2048;
+
+  MOZ_ASSERT(uintptr_t(aPtr) < uintptr_t(&aPtr) ||
+             uintptr_t(aPtr) > uintptr_t(&aPtr) + kFuzz);
 #endif
 }
 
 static inline nsCString
 AsLiteralCString(const char* aStr)
 {
   AssertNotMallocAllocated(aStr);
   AssertNotStackAllocated(aStr);