Skip to content

Exclude non-user portions of the main thread stack from stack scanning on 32 bits. #46034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Exclude non-user portions of the main thread stack from stack scanning on 32 bits. #46034

wants to merge 1 commit into from
+57 −16

Conversation

justinmichaud
Copy link
Contributor

@justinmichaud justinmichaud commented May 29, 2025 •
edited by webkit-early-warning-system
Loading

3f2163b

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 NOBODY (OOPS!).

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/WTF/wtf/StackBounds.cpp:
(WTF::StackBounds::setBottomOfMainThreadMain):
(WTF::StackBounds::currentThreadStackBoundsInternal):
* Source/WTF/wtf/StackBounds.h:
* Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:
* Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:

e63393f

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug loading 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@justinmichaud justinmichaud requested review from a team as code owners May 29, 2025 02:50
@justinmichaud justinmichaud self-assigned this May 29, 2025
@justinmichaud justinmichaud added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label May 29, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 29, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 3f2163b)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ⏳ 🧪 ios-wk2 ✅ 🧪 api-mac   🧪 api-wpe
⏳ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ⏳ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ⏳ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ⏳ 🛠 vision ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
⏳ 🛠 vision-sim ✅ 🧪 mac-wk2-stress   🧪 api-gtk
⏳ 🧪 vision-wk2 ⏳ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
✅ 🛠 tv ⏳ 🛠 mac-safer-cpp ❌ 🛠 jsc-armv7
⏳ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
⏳ 🛠 watch
⏳ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 29, 2025
@justinmichaud justinmichaud force-pushed the eng/Exclude-non-user-portions-of-the-main-thread-stack-from-stack-scanning-on-32-bits branch from 3f2163b to 507ad76 Compare May 29, 2025 03:55
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 29, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 507ad76)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
💥 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ❌ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@justinmichaud justinmichaud mentioned this pull request May 29, 2025
Open
Constellation
Constellation reviewed May 31, 2025
View reviewed changes
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this optionally: if this is not called, use environ. And when this is called and if this narrows down the stack further, do it with this.

@justinmichaud justinmichaud force-pushed the eng/Exclude-non-user-portions-of-the-main-thread-stack-from-stack-scanning-on-32-bits branch from 507ad76 to 5fbc49d Compare June 23, 2025 20:15
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 23, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 5fbc49d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ⏳ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ❌ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt   🧪 mac-wk1 ⏳ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2   🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
  🧪 vision-wk2   🧪 mac-intel-wk2 ⏳ 🛠 playstation
⏳ 🛠 tv ⏳ 🛠 mac-safer-cpp ❌ 🛠 jsc-armv7
⏳ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
⏳ 🛠 watch
✅ 🛠 watch-sim

@justinmichaud justinmichaud force-pushed the eng/Exclude-non-user-portions-of-the-main-thread-stack-from-stack-scanning-on-32-bits branch from 5fbc49d to da4a741 Compare June 23, 2025 20:18
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 23, 2025 •
edited
Loading

EWS run on previous version of this PR (hash da4a741)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ⏳ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@justinmichaud justinmichaud removed the merging-blocked Applied to prevent a change from being merged label Jun 24, 2025
@justinmichaud
Exclude non-user portions of the main thread stack from stack scannin…
e63393f 
…g on 32 bits.

https://bugs.webkit.org/show_bug.cgi?id=293720

Reviewed by NOBODY (OOPS!).

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/WTF/wtf/StackBounds.cpp:
(WTF::StackBounds::setBottomOfMainThreadMain):
(WTF::StackBounds::currentThreadStackBoundsInternal):
* Source/WTF/wtf/StackBounds.h:
* Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:
* Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:
@justinmichaud justinmichaud force-pushed the eng/Exclude-non-user-portions-of-the-main-thread-stack-from-stack-scanning-on-32-bits branch from da4a741 to e63393f Compare June 27, 2025 19:51
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 27, 2025 •
edited
Loading

EWS run on current version of this PR (hash e63393f)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug loading 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

justinmichaud
justinmichaud commented Jun 27, 2025
View reviewed changes
Source/JavaScriptCore/runtime/VM.h
bool currentThreadIsHoldingAPILock() const { return m_apiLock->currentThreadIsHoldingLock(); }
bool currentThreadIsHoldingAPILock() const
{
#if ASSERT_ENABLED || OS(LINUX)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have extended this to release builds to ensure this doesn't cause new UAFs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But only on non-Apple platforms

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Constellation Constellation Constellation left review comments

Assignees

@justinmichaud justinmichaud

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. merging-blocked Applied to prevent a change from being merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justinmichaud @webkit-early-warning-system @Constellation @webkit-ews-buildbot