Skip to content

Commit 0f53210

Browse files
committed
Exclude non-user portions of the main thread stack from stack scanning on 32 bits.
https://bugs.webkit.org/show_bug.cgi?id=293720 Reviewed by Yusuke Suzuki. On 32-bit armv7/linux we run into an issue where the environment strings located at the bottom of the stack, as well as some random bits inside the libc portion of the stack are interpreted as cells pointing into the heap, keeping actually dead objects alive. Yusuke previously attempted to fix this by excluding environ and before, but there are additional fake roots only a few hundred bytes below that. This patch excludes the caller of main entirely. * Source/JavaScriptCore/jsc.cpp: (jscmain): * Source/JavaScriptCore/runtime/JSLock.cpp: (JSC::JSLock::didAcquireLock): * Source/WTF/wtf/StackBounds.cpp: (WTF::StackBounds::setBottomOfMainThreadMain): (WTF::StackBounds::currentThreadStackBoundsInternal): * Source/WTF/wtf/StackBounds.h: (WTF::StackBounds::checkConsistency const): * Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp: (WebKit::WebProcessMain): * Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp: (WebKit::WebProcessMain):
1 parent dd775c7 commit 0f53210

6 files changed

Lines changed: 40 additions & 8 deletions

File tree

Source/JavaScriptCore/jsc.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3869,6 +3869,7 @@ int jscmain(int argc, char** argv)
38693869
// Need to override and enable restricted options before we start parsing options below.
38703870
Config::enableRestrictedOptions();
38713871

3872+
WTF::StackBounds::setBottomOfMainThreadMain(__builtin_frame_address(0));
38723873
WTF::initializeMainThread();
38733874

38743875
// Note that the options parsing can affect VM creation, and thus

Source/JavaScriptCore/runtime/JSLock.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,10 @@ void JSLock::didAcquireLock()
169169
samplingProfiler->noticeJSLockAcquisition();
170170
}
171171
#endif
172+
#if ASSERT_ENABLED
173+
StackBounds::currentThreadStackBounds(); // Check stack bounds consistency.
174+
#endif
175+
172176
}
173177

174178
void JSLock::unlock()

Source/WTF/wtf/StackBounds.cpp

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
#include "config.h"
2222
#include <wtf/StackBounds.h>
2323

24+
#include "MainThread.h"
25+
2426
#if OS(DARWIN)
2527

2628
#include <pthread.h>
@@ -46,6 +48,18 @@
4648

4749
namespace WTF {
4850

51+
#if !CPU(ADDRESS64)
52+
static std::atomic<void*> bottomOfMainThreadMain = nullptr;
53+
#endif
54+
55+
void StackBounds::setBottomOfMainThreadMain([[maybe_unused]] void* stack)
56+
{
57+
#if !CPU(ADDRESS64)
58+
RELEASE_ASSERT(bottomOfMainThreadMain == nullptr);
59+
bottomOfMainThreadMain = stack;
60+
#endif
61+
}
62+
4963
#if OS(DARWIN)
5064

5165
StackBounds StackBounds::newThreadStackBounds(PlatformThreadHandle thread)
@@ -135,15 +149,23 @@ StackBounds StackBounds::currentThreadStackBoundsInternal()
135149

136150
static char** oldestEnviron = environ;
137151

138-
// In 32bit architecture, it is possible that environment variables are having a characters which looks like a pointer,
139-
// and conservative GC will find it as a live pointer. We would like to avoid that to precisely exclude non user stack
140-
// data region from this stack bounds. As the article (https://lwn.net/Articles/631631/) and the elf loader implementation
141-
// explain how Linux main thread stack is organized, environment variables vector is placed on the stack, so we can exclude
142-
// environment variables if we use `environ` global variable as a origin of the stack.
152+
// On 32bit, it is possible that environment variables and other random data in libc have bytes which look like a pointer,
153+
// and conservative GC will find it as a live pointer. We would like to avoid that, so we exclude the non-user stack
154+
// data region from these stack bounds. This article explains how the main thread looks (https://lwn.net/Articles/631631/).
155+
//
156+
// First, we exclude environment variables by using `environ` global variable as a origin of the stack.
143157
// But `setenv` / `putenv` may alter `environ` variable's content. So we record the oldest `environ` variable content, and use it.
158+
144159
StackBounds stackBounds { origin, bound };
145160
if (stackBounds.contains(oldestEnviron))
146161
stackBounds = { oldestEnviron, bound };
162+
163+
// Then, we exclude any greater region based on manual calls to setBottomOfMainThreadMain.
164+
#if !CPU(ADDRESS64)
165+
if (stackBounds.contains(bottomOfMainThreadMain))
166+
stackBounds = { bottomOfMainThreadMain, bound };
167+
#endif
168+
147169
return stackBounds;
148170
}
149171
#endif

Source/WTF/wtf/StackBounds.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ class StackBounds {
6363
result.checkConsistency();
6464
return result;
6565
}
66+
WTF_EXPORT_PRIVATE static void setBottomOfMainThreadMain(void*);
6667

6768
void* origin() const
6869
{
@@ -142,10 +143,10 @@ class StackBounds {
142143

143144
void checkConsistency() const
144145
{
145-
#if ASSERT_ENABLED
146+
#if ASSERT_ENABLED || OS(LINUX)
146147
void* currentPosition = currentStackPointer();
147-
ASSERT(m_origin != m_bound);
148-
ASSERT(currentPosition < m_origin && currentPosition > m_bound);
148+
RELEASE_ASSERT(m_origin != m_bound);
149+
RELEASE_ASSERT(currentPosition < m_origin && currentPosition > m_bound);
149150
#endif
150151
}
151152

Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "WebProcess.h"
3232
#include <WebCore/GtkVersioning.h>
3333
#include <libintl.h>
34+
#include <wtf/StackBounds.h>
3435

3536
#if PLATFORM(X11)
3637
#include <X11/Xlib.h>
@@ -95,6 +96,7 @@ int WebProcessMain(int argc, char** argv)
9596
// This call needs to happen before any threads begin execution
9697
unsetenv("GTK_THEME");
9798

99+
WTF::StackBounds::setBottomOfMainThreadMain(__builtin_frame_address(0));
98100
return AuxiliaryProcessMain<WebProcessMainGtk>(argc, argv);
99101
}
100102

Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "AuxiliaryProcessMain.h"
3131
#include "WebProcess.h"
3232
#include <glib.h>
33+
#include <wtf/StackBounds.h>
3334

3435
#if USE(GCRYPT)
3536
#include <pal/crypto/gcrypt/Initialization.h>
@@ -72,6 +73,7 @@ class WebProcessMainWPE final : public AuxiliaryProcessMainBase<WebProcess> {
7273

7374
int WebProcessMain(int argc, char** argv)
7475
{
76+
WTF::StackBounds::setBottomOfMainThreadMain(__builtin_frame_address(0));
7577
return AuxiliaryProcessMain<WebProcessMainWPE>(argc, argv);
7678
}
7779

0 commit comments

Comments
 (0)