Reland "[wasm] Disallow late enabling of trap handlers"

This is a reland of bcb0a7c5.
Data races detected by TSan are fixed by using (relaxed) atomic
updates.

Original change's description:
> [wasm] Disallow late enabling of trap handlers
>
> It's dangerous if trap handlers are enabled after we already used the
> information whether they are enabled or not.
> This CL checks for such misbehaviour by remembering whether
> {IsTrapHandlerEnabled} was already called, and disallowing
> {EnableTrapHandler} afterwards. Also, calling {EnableTrapHandler}
> multiple times is disallowed now.
>
> The trap handler tests are changed to only enable trap handlers once,
> and to do that before allocating wasm memory or generating code.
>
> R=ahaas@chromium.org
>
> Bug: v8:11017
> Change-Id: Ib2256bb8435efd914c12769cedd4a0051052aeef
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2494935
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#70750}

Bug: v8:11017
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_rel_ng
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_isolates_rel_ng
Change-Id: I24299c433ffa3ce31e2aac12134dc03f30609da2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2498683
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70761}

src/trap-handler/handler-outside.cc

@@ -249,9 +249,19 @@ bool RegisterDefaultTrapHandler() { return false; }
 void RemoveTrapHandler() {}
 #endif
 
-bool g_is_trap_handler_enabled;
+bool g_is_trap_handler_enabled{false};
+std::atomic<bool> g_can_enable_trap_handler{true};
 
 bool EnableTrapHandler(bool use_v8_handler) {
+  // We should only enable the trap handler once, and before any call to
+  // {IsTrapHandlerEnabled}. Enabling the trap handler late can lead to problems
+  // because code or objects might have been generated under the assumption that
+  // trap handlers are disabled.
+  bool can_enable =
+      g_can_enable_trap_handler.exchange(false, std::memory_order_relaxed);
+  if (!can_enable) {
+    FATAL("EnableTrapHandler called twice, or after IsTrapHandlerEnabled");
+  }
   if (!V8_TRAP_HANDLER_SUPPORTED) {
     return false;
   }

src/trap-handler/trap-handler.h

@@ -8,6 +8,8 @@
 #include <stdint.h>
 #include <stdlib.h>
 
+#include <atomic>
+
 #include "src/base/build_config.h"
 #include "src/common/globals.h"
 #include "src/flags/flags.h"
@@ -64,7 +66,17 @@ void V8_EXPORT_PRIVATE ReleaseHandlerData(int index);
 #define THREAD_LOCAL __thread
 #endif
 
+// Initially false, set to true if when trap handlers are enabled. Never goes
+// back to false then.
 extern bool g_is_trap_handler_enabled;
+
+// Initially true, set to false when either {IsTrapHandlerEnabled} or
+// {EnableTrapHandler} is called to prevent calling {EnableTrapHandler}
+// repeatedly, or after {IsTrapHandlerEnabled}. Needs to be atomic because
+// {IsTrapHandlerEnabled} can be called from any thread. Updated using relaxed
+// semantics, since it's not used for synchronization.
+extern std::atomic<bool> g_can_enable_trap_handler;
+
 // Enables trap handling for WebAssembly bounds checks.
 //
 // use_v8_handler indicates that V8 should install its own handler
@@ -73,6 +85,13 @@ V8_EXPORT_PRIVATE bool EnableTrapHandler(bool use_v8_handler);
 
 inline bool IsTrapHandlerEnabled() {
   DCHECK_IMPLIES(g_is_trap_handler_enabled, V8_TRAP_HANDLER_SUPPORTED);
+  // Disallow enabling the trap handler after retrieving the current value.
+  // Re-enabling them late can produce issues because code or objects might have
+  // been generated under the assumption that trap handlers are disabled.
+  // Note: We test before setting to avoid contention by an unconditional write.
+  if (g_can_enable_trap_handler.load(std::memory_order_relaxed)) {
+    g_can_enable_trap_handler.store(false, std::memory_order_relaxed);
+  }
   return g_is_trap_handler_enabled;
 }
 

test/unittests/wasm/trap-handler-x64-unittest.cc

@@ -80,7 +80,8 @@ class TrapHandlerTest : public TestWithIsolate,
                         public ::testing::WithParamInterface<TrapHandlerStyle> {
  protected:
   void SetUp() override {
-    CHECK(trap_handler::EnableTrapHandler(false));
+    InstallFallbackHandler();
+    SetupTrapHandler(GetParam());
     backing_store_ = BackingStore::AllocateWasmMemory(i_isolate(), 1, 1,
                                                       SharedFlag::kNotShared);
     CHECK(backing_store_);
@@ -93,7 +94,9 @@ class TrapHandlerTest : public TestWithIsolate,
                                       GetRandomMmapAddr());
 
     InitRecoveryCode();
+  }
 
+  void InstallFallbackHandler() {
 #if V8_OS_LINUX || V8_OS_MACOSX || V8_OS_FREEBSD
     // Set up a signal handler to recover from the expected crash.
     struct sigaction action;
@@ -197,13 +200,13 @@ class TrapHandlerTest : public TestWithIsolate,
   }
 #endif
 
- public:
   void SetupTrapHandler(TrapHandlerStyle style) {
     bool use_default_handler = style == kDefault;
     g_use_as_first_chance_handler = !use_default_handler;
     CHECK(v8::V8::EnableWebAssemblyTrapHandler(use_default_handler));
   }
 
+ public:
   void GenerateSetThreadInWasmFlagCode(MacroAssembler* masm) {
     masm->Move(scratch,
                i_isolate()->thread_local_top()->thread_in_wasm_flag_address_,
@@ -282,7 +285,6 @@ TEST_P(TrapHandlerTest, TestTrapHandlerRecovery) {
   CodeDesc desc;
   masm.GetCode(nullptr, &desc);
 
-  SetupTrapHandler(GetParam());
   trap_handler::ProtectedInstructionData protected_instruction{crash_offset,
                                                                recovery_offset};
   trap_handler::RegisterHandlerData(reinterpret_cast<Address>(desc.buffer),
@@ -314,8 +316,6 @@ TEST_P(TrapHandlerTest, TestReleaseHandlerData) {
       reinterpret_cast<Address>(desc.buffer), desc.instr_size, 1,
       &protected_instruction);
 
-  SetupTrapHandler(GetParam());
-
   ExecuteBuffer();
 
   // Deregister from the trap handler. The trap handler should not do the
@@ -345,8 +345,6 @@ TEST_P(TrapHandlerTest, TestNoThreadInWasmFlag) {
   trap_handler::RegisterHandlerData(reinterpret_cast<Address>(desc.buffer),
                                     desc.instr_size, 1, &protected_instruction);
 
-  SetupTrapHandler(GetParam());
-
   ExecuteExpectCrash(buffer_.get());
 }
 
@@ -373,8 +371,6 @@ TEST_P(TrapHandlerTest, TestCrashInWasmNoProtectedInstruction) {
   trap_handler::RegisterHandlerData(reinterpret_cast<Address>(desc.buffer),
                                     desc.instr_size, 1, &protected_instruction);
 
-  SetupTrapHandler(GetParam());
-
   ExecuteExpectCrash(buffer_.get());
 }
 
@@ -401,8 +397,6 @@ TEST_P(TrapHandlerTest, TestCrashInWasmWrongCrashType) {
   trap_handler::RegisterHandlerData(reinterpret_cast<Address>(desc.buffer),
                                     desc.instr_size, 1, &protected_instruction);
 
-  SetupTrapHandler(GetParam());
-
 #if V8_OS_POSIX
   // On Posix, the V8 default trap handler does not register for SIGFPE,
   // therefore the thread-in-wasm flag is never reset in this test. We
@@ -462,8 +456,6 @@ TEST_P(TrapHandlerTest, TestCrashInOtherThread) {
   trap_handler::RegisterHandlerData(reinterpret_cast<Address>(desc.buffer),
                                     desc.instr_size, 1, &protected_instruction);
 
-  SetupTrapHandler(GetParam());
-
   CodeRunner runner(this, buffer_.get());
   CHECK(!GetThreadInWasmFlag());
   // Set the thread-in-wasm flag manually in this thread.