[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/+/2494935Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70750}

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;
+bool g_can_enable_trap_handler = true;
 
 bool EnableTrapHandler(bool use_v8_handler) {
+  if (!g_can_enable_trap_handler) {
+    // Enabling the trap handler after IsTrapHandlerEnabled was called can lead
+    // to problems because code or objects might have been generated under the
+    // assumption that trap handlers are disabled.
+    FATAL("EnableTrapHandler called after IsTrapHandlerEnabled");
+  }
+  // We should only enable the trap handler once.
+  g_can_enable_trap_handler = false;
+  CHECK(!g_is_trap_handler_enabled);
   if (!V8_TRAP_HANDLER_SUPPORTED) {
     return false;
   }

src/trap-handler/trap-handler.h

@@ -65,6 +65,8 @@ void V8_EXPORT_PRIVATE ReleaseHandlerData(int index);
 #endif
 
 extern bool g_is_trap_handler_enabled;
+extern 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 +75,10 @@ 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.
+  g_can_enable_trap_handler = false;
   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.