Bug 232165 – JITReservation initializeJITPageReservation() overwrites g_wtfConfig with USE_SYSTEM_MALLOC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Pascal Abresch]

This was discovered in the haiku port because of an assertion in Source/WTF/wtf/threads/Signals.cpp:343

Reproduced with https://github.com/haiku/haikuwebkit/commit/889085c4ff8e7954ca830cb3b54b33113e707718
Which is based on https://github.com/WebKit/WebKit/commit/b683340bee5c998a212c2918cfa0af5389579a99

This is the patch used to fix the haiku build
diff --git a/Source/JavaScriptCore/jit/ExecutableAllocator.cpp b/Source/JavaScriptCore/jit/ExecutableAllocator.cpp
index 31476313ba..669915502a 100644
--- a/Source/JavaScriptCore/jit/ExecutableAllocator.cpp
+++ b/Source/JavaScriptCore/jit/ExecutableAllocator.cpp
@@ -403,7 +403,7 @@ static ALWAYS_INLINE JITReservation initializeJITPageReservation()
         g_jscConfig.startExecutableMemory = tagCodePtr<ExecutableMemoryPtrTag>(reservation.base);
         g_jscConfig.endExecutableMemory = tagCodePtr<ExecutableMemoryPtrTag>(reservationEnd);
 
-#if ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD)
+#if !USE(SYSTEM_MALLOC) && ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD)
         WebConfig::g_config[0] = bitwise_cast<uintptr_t>(reservation.base);
         WebConfig::g_config[1] = bitwise_cast<uintptr_t>(reservationEnd);
 #endif

[Pascal Abresch]

Full patch

From 9207543134385c8fb52e1e5fc48b491d377d9c2e Mon Sep 17 00:00:00 2001
From: waddlesplash <[e-mail]>
Date: Fri, 22 Oct 2021 14:27:12 -0400
Subject: [PATCH] ExecutableAllocator: Do not store things in g_config when
 USE(SYSTEM_MALLOC).

Following 41bdcb765f0f1e658c943b2bbf778e8b33fb783b, two additional slots were added
to g_config in order to store these pointers for use in bmalloc and Gigacage. However, when
USE(SYSTEM_MALLOC) is enabled, there are no slots reserved for Gigacage, and so this collided
with g_wtfConfig and overwrote data there instead.

This should fix crashes seen on Haiku, which enables USE(SYSTEM_MALLOC).
---
 Source/JavaScriptCore/jit/ExecutableAllocator.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Source/JavaScriptCore/jit/ExecutableAllocator.cpp b/Source/JavaScriptCore/jit/ExecutableAllocator.cpp
index 31476313ba..387c890818 100644
--- a/Source/JavaScriptCore/jit/ExecutableAllocator.cpp
+++ b/Source/JavaScriptCore/jit/ExecutableAllocator.cpp
@@ -403,7 +403,7 @@ static ALWAYS_INLINE JITReservation initializeJITPageReservation()
         g_jscConfig.startExecutableMemory = tagCodePtr<ExecutableMemoryPtrTag>(reservation.base);
         g_jscConfig.endExecutableMemory = tagCodePtr<ExecutableMemoryPtrTag>(reservationEnd);
 
-#if ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD)
+#if !USE(SYSTEM_MALLOC) && ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD)
         WebConfig::g_config[0] = bitwise_cast<uintptr_t>(reservation.base);
         WebConfig::g_config[1] = bitwise_cast<uintptr_t>(reservationEnd);
 #endif
-- 
2.30.2

[Radar WebKit Bug Importer]

<rdar://problem/84819759>

[Jonathan Bedard]

(In reply to Pascal Abresch from comment #1)
> ...

Could we get this uploaded as an attachment so it triggers EWS? `Tools/Scripts/webkit-patch upload` will do everything for you, otherwise, uploading the patch as an attachment manually and marking it as a patch should also trigger EWS.

[Chris Dumez]

I recommend reading https://webkit.org/contributing-code/.

[Pascal Abresch]

Created attachment 446888 [details]
Patch

[Pascal Abresch]

Sorry for the delay, I was unavailable out of town.
I have tried to upload the patch with the script now, i am not sure if it's entirely correct now, still learning the webkit tooling (The upload-patch script used links2 to show the patch after it did not find vi... that was a bit strange to me)

[Yusuke Suzuki]

Comment on attachment 446888 [details]
Patch

Please do not modify the existing ChangeLog's entries. I think probably your editor has some configuration which removes trailing spaces etc. automatically.

[Pascal Abresch]

Created attachment 446899 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 446899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446899&action=review

r=me with comment.

> Source/JavaScriptCore/ChangeLog:8
> +        Following 41bdcb765f0f1e658c943b2bbf778e8b33fb783b, two additional slots were added

Please use revision number instead of git hash.

[Pascal Abresch]

Created attachment 447027 [details]
Patch

[Pascal Abresch]

I wasn't quite sure what the format is for the revision number, I hope I picked the correct one.

[Yusuke Suzuki]

Comment on attachment 447027 [details]
Patch

r=me. We typically use rREVNUMBER (in this case, r281910).

[Pascal Abresch]

Created attachment 447039 [details]
Patch

[EWS]

Committed r286969 (245191@main): <https://commits.webkit.org/245191@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447039 [details].