REGRESSION(macOS Ventura): OpenAudible, Eclipse and other Java applications crash when using WebKit #7725

Constellation · 2022-12-16T00:17:59Z

`0b27858`

REGRESSION(macOS Ventura): OpenAudible, Eclipse and other Java applications crash when using WebKit
https://bugs.webkit.org/show_bug.cgi?id=247387
rdar://101892715

Reviewed by Mark Lam.

x64 binary can change unmapped memory region, breaking JSC's assumption on mmap-returned memory address.
By modifying PAGEZERO address, x64 application can change the start of mmap-returned memory address.
By default, it is 4GB, but it can be anything larger than 4KB (On the other hand, ARM64 enforces it
to 4GB at minimum). This patch updates the number for filtering.

* Source/JavaScriptCore/tools/Integrity.h:
(JSC::Integrity::isSanePointer):
* Source/bmalloc/libpas/src/libpas/pas_root.c:
(pas_root_visit_conservative_candidate_pointers_in_address_range):

Canonical link: https://commits.webkit.org/257973@main

086640f

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-AS-debug~~	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
~~🛠 🧪 merge~~	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests
✅ 🛠 🧪 unsafe-merge

webkit-early-warning-system · 2022-12-16T00:20:38Z

EWS run on previous version of this PR (hash 0f63dbf)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	⏳ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk2	✅ 🛠 jsc-armv7
⏳ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	⏳ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

Constellation · 2022-12-16T00:29:05Z

Let me make it pending. I think this 4GB assumption is right for most of applications. And crashing application has very special thing making this invalid. We should change this logic only when this specific logic is used. Investigating.

webkit-early-warning-system · 2022-12-16T00:52:54Z

EWS run on previous version of this PR (hash 95ae71c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	⏳ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	⏳ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

aproskuryakov · 2022-12-16T00:56:28Z

Is there a reason to not explain what the PR is actually fixing? We usually care about the symptom more than about how the bug is fixed, so the title should explain the symptom.

MenloDorian

r=me

MenloDorian · 2022-12-16T01:00:48Z

Source/JavaScriptCore/tools/Integrity.h

Should we just change these to 4ULL instead of static_cast<uintptr_t>(4)?

Actually either is fine because it is inside CPU(ADDRESS64).

Constellation · 2022-12-16T01:15:08Z

Is there a reason to not explain what the PR is actually fixing? We usually care about the symptom more than about how the bug is fixed, so the title should explain the symptom.

I think "x64 binary can change unmapped memory region" is symptom (And changing the numbers for the check is the fix). But I can change a bit more :).

webkit-early-warning-system · 2022-12-16T01:17:03Z

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

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

aproskuryakov · 2022-12-16T01:19:28Z

That's the cause of the user observable issue. There are more people who aren't JSC developers reading bug and PR titles, so we should optimize for them. I think that the bug title is great (REGRESSION (macOS Ventura): OpenAudible, Eclipse and other Java applications crash when using WebKit).

Constellation · 2022-12-16T01:22:37Z

That's the cause of the user observable issue. There are more people who aren't JSC developers reading bug and PR titles, so we should optimize for them. I think that the bug title is great (REGRESSION (macOS Ventura): OpenAudible, Eclipse and other Java applications crash when using WebKit).

I feel this looks very hard to see what the change is for JSC developers reading JSC changes, but if you have strong opinion, I'm not opposing to it strongly.

aproskuryakov · 2022-12-16T01:26:45Z

The first line of commit message is the bug title by convention, and we certainly shouldn't retitle this bug. I am not going to argue this any further if you disagree, but I do feel strongly about this convention.

webkit-early-warning-system · 2022-12-16T01:51:27Z

EWS run on current version of this PR (hash 086640f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-AS-debug~~	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
~~🛠 🧪 merge~~	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests
✅ 🛠 🧪 unsafe-merge

…ations crash when using WebKit https://bugs.webkit.org/show_bug.cgi?id=247387 rdar://101892715 Reviewed by Mark Lam. x64 binary can change unmapped memory region, breaking JSC's assumption on mmap-returned memory address. By modifying PAGEZERO address, x64 application can change the start of mmap-returned memory address. By default, it is 4GB, but it can be anything larger than 4KB (On the other hand, ARM64 enforces it to 4GB at minimum). This patch updates the number for filtering. * Source/JavaScriptCore/tools/Integrity.h: (JSC::Integrity::isSanePointer): * Source/bmalloc/libpas/src/libpas/pas_root.c: (pas_root_visit_conservative_candidate_pointers_in_address_range): Canonical link: https://commits.webkit.org/257973@main

webkit-commit-queue · 2022-12-16T04:38:57Z

Committed 257973@main (0b27858): https://commits.webkit.org/257973@main

Reviewed commits have been landed. Closing PR #7725 and removing active labels.

Constellation requested a review from a team as a code owner December 16, 2022 00:18

Constellation self-assigned this Dec 16, 2022

Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Dec 16, 2022

Constellation marked this pull request as draft December 16, 2022 00:29

Constellation force-pushed the eng/REGRESSION-macOS-Ventura-OpenAudible-Eclipse-and-other-Java-applications-crash-when-using-WebKit branch from 0f63dbf to 95ae71c Compare December 16, 2022 00:49

Constellation changed the title ~~x64 macOS Ventura mmap can return memory address less than 4GB~~ x64 binary can change unmapped memory region Dec 16, 2022

Constellation marked this pull request as ready for review December 16, 2022 00:49

MenloDorian approved these changes Dec 16, 2022

View reviewed changes

Constellation force-pushed the eng/REGRESSION-macOS-Ventura-OpenAudible-Eclipse-and-other-Java-applications-crash-when-using-WebKit branch from 95ae71c to 3a268ee Compare December 16, 2022 01:16

Constellation changed the title ~~x64 binary can change unmapped memory region~~ x64 binary can change unmapped memory region, breaking JSC's assumption on mmap-returned memory address Dec 16, 2022

Constellation force-pushed the eng/REGRESSION-macOS-Ventura-OpenAudible-Eclipse-and-other-Java-applications-crash-when-using-WebKit branch from 3a268ee to 086640f Compare December 16, 2022 01:47

Constellation changed the title ~~x64 binary can change unmapped memory region, breaking JSC's assumption on mmap-returned memory address~~ REGRESSION(macOS Ventura): OpenAudible, Eclipse and other Java applications crash when using WebKit Dec 16, 2022

webkit-early-warning-system force-pushed the eng/REGRESSION-macOS-Ventura-OpenAudible-Eclipse-and-other-Java-applications-crash-when-using-WebKit branch from 086640f to 0b27858 Compare December 16, 2022 04:38

webkit-early-warning-system merged commit 0b27858 into WebKit:main Dec 16, 2022

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 16, 2022

Constellation deleted the eng/REGRESSION-macOS-Ventura-OpenAudible-Eclipse-and-other-Java-applications-crash-when-using-WebKit branch December 16, 2022 04:43

REGRESSION(macOS Ventura): OpenAudible, Eclipse and other Java applications crash when using WebKit #7725

REGRESSION(macOS Ventura): OpenAudible, Eclipse and other Java applications crash when using WebKit #7725

Uh oh!

Conversation

Constellation commented Dec 16, 2022 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

0b27858

Uh oh!

webkit-early-warning-system commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Constellation commented Dec 16, 2022

Uh oh!

webkit-early-warning-system commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aproskuryakov commented Dec 16, 2022

Uh oh!

MenloDorian left a comment

Choose a reason for hiding this comment

Uh oh!

MenloDorian Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

Constellation Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

Constellation commented Dec 16, 2022

Uh oh!

webkit-early-warning-system commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aproskuryakov commented Dec 16, 2022

Uh oh!

Constellation commented Dec 16, 2022

Uh oh!

aproskuryakov commented Dec 16, 2022

Uh oh!

webkit-early-warning-system commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Constellation commented Dec 16, 2022 •

edited by webkit-early-warning-system

Loading

`0b27858`

webkit-early-warning-system commented Dec 16, 2022 •

edited

Loading

webkit-early-warning-system commented Dec 16, 2022 •

edited

Loading

webkit-early-warning-system commented Dec 16, 2022 •

edited

Loading

webkit-early-warning-system commented Dec 16, 2022 •

edited

Loading