Bug 1483865: Make NotStackAllocated assertion a bit safer. r=froydnj
☠☠ backed out by dfc1e2e0156d ☠ ☠
authorKris Maglione <maglione.k@gmail.com>
Thu, 16 Aug 2018 11:35:50 -0700
changeset 432871 a5f51c76930c49160bf5e909301d8e7f1a83e379
parent 432870 48213c5c25e9aa9389375e4e71c1a58567d37ca2
child 432872 dfc1e2e0156dde85cc9ca40a1e4ba3bf2d1bb60d
push id106903
push usermaglione.k@gmail.com
push dateWed, 22 Aug 2018 18:19:26 +0000
treeherdermozilla-inbound@a5f51c76930c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1483865
milestone63.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 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,24 +475,41 @@ 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));
-#endif
+  // 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.
+  static constexpr size_t kFuzz = 2048;
+
+  MOZ_ASSERT(uintptr_t(aPtr) < uintptr_t(&aPtr) ||
+             uintptr_t(aPtr) > uintptr_t(&aPtr) + kFuzz);
 }
 
 static inline nsCString
 AsLiteralCString(const char* aStr)
 {
   AssertNotMallocAllocated(aStr);
   AssertNotStackAllocated(aStr);