[wasm] do not register trap handler data for previously registered code

Previously, we would blindly register new handler data, leading to us leaking
the old handler data. This meant we could then end up with overlapping handler
data where the instruction offset and landing pads didn't line up right.

Bug: v8:6841
Change-Id: Iedcd75925b8d9d59c8f9accf288cae954fdc568f
Reviewed-on: https://chromium-review.googlesource.com/677632
Commit-Queue: Eric Holk <eholk@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Mircea Trofin <mtrofin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48144}

src/heap/heap.cc

@@ -51,6 +51,7 @@
 #include "src/snapshot/serializer-common.h"
 #include "src/snapshot/snapshot.h"
 #include "src/tracing/trace-event.h"
+#include "src/trap-handler/trap-handler.h"
 #include "src/utils-inl.h"
 #include "src/utils.h"
 #include "src/v8.h"
@@ -3110,6 +3111,12 @@ AllocationResult Heap::CopyCode(Code* code) {
   DCHECK(!memory_allocator()->code_range()->valid() ||
          memory_allocator()->code_range()->contains(code->address()) ||
          obj_size <= code_space()->AreaSize());
+
+  // Clear the trap handler index since they can't be shared between code. We
+  // have to do this before calling Relocate becauase relocate would adjust the
+  // base pointer for the old code.
+  new_code->set_trap_handler_index(Smi::FromInt(trap_handler::kInvalidIndex));
+
   new_code->Relocate(new_addr - old_addr);
   // We have to iterate over the object and process its pointers when black
   // allocation is on.

src/snapshot/code-serializer.cc

@@ -254,10 +254,21 @@ void WasmCompiledModuleSerializer::SerializeCodeObject(
   Code::Kind kind = code_object->kind();
   switch (kind) {
     case Code::WASM_FUNCTION:
-    case Code::JS_TO_WASM_FUNCTION:
+    case Code::JS_TO_WASM_FUNCTION: {
+      // Because the trap handler index is not meaningful across copies and
+      // serializations, we need to serialize it as kInvalidIndex. We do this by
+      // saving the old value, setting the index to kInvalidIndex and then
+      // restoring the old value.
+      const int old_trap_handler_index =
+          code_object->trap_handler_index()->value();
+      code_object->set_trap_handler_index(
+          Smi::FromInt(trap_handler::kInvalidIndex));
+
       // Just serialize the code_object.
       SerializeGeneric(code_object, how_to_code, where_to_point);
+      code_object->set_trap_handler_index(Smi::FromInt(old_trap_handler_index));
       break;
+    }
     case Code::WASM_INTERPRETER_ENTRY:
     case Code::WASM_TO_JS_FUNCTION:
       // Serialize the illegal builtin instead. On instantiation of a

src/trap-handler/handler-outside.cc

@@ -111,7 +111,7 @@ int RegisterHandlerData(void* base, size_t size,
       new_size = int_max;
     }
     if (new_size == gNumCodeObjects) {
-      return -1;
+      return kInvalidIndex;
     }
 
     // Now that we know our new size is valid, we can go ahead and realloc the
@@ -145,11 +145,16 @@ int RegisterHandlerData(void* base, size_t size,
     gCodeObjects[i].code_info = data;
     return static_cast<int>(i);
   } else {
-    return -1;
+    return kInvalidIndex;
   }
 }
 
 void ReleaseHandlerData(int index) {
+  if (index == kInvalidIndex) {
+    return;
+  }
+  DCHECK_GE(index, 0);
+
   // Remove the data from the global list if it's there.
   CodeProtectionInfo* data = nullptr;
   {

src/trap-handler/trap-handler.h

@@ -38,6 +38,8 @@ struct ProtectedInstructionData {
   intptr_t landing_offset;
 };
 
+const int kInvalidIndex = -1;
+
 /// Adjusts the base code pointer.
 void UpdateHandlerDataCodePointer(int index, void* base);
 

src/wasm/wasm-module.cc

@@ -109,7 +109,7 @@ static void InstanceFinalizer(const v8::WeakCallbackInfo<void>& data) {
       int index = code->trap_handler_index()->value();
       if (index >= 0) {
         trap_handler::ReleaseHandlerData(index);
-        code->set_trap_handler_index(Smi::FromInt(-1));
+        code->set_trap_handler_index(Smi::FromInt(trap_handler::kInvalidIndex));
       }
     }
   }
@@ -317,6 +317,11 @@ void UnpackAndRegisterProtectedInstructions(Isolate* isolate,
       continue;
     }
 
+    if (code->trap_handler_index()->value() != trap_handler::kInvalidIndex) {
+      // This function has already been registered.
+      continue;
+    }
+
     const intptr_t base = reinterpret_cast<intptr_t>(code->entry());
 
     Zone zone(isolate->allocator(), "Wasm Module");

test/cctest/wasm/wasm-run-utils.cc

@@ -472,6 +472,16 @@ WasmFunctionCompiler::WasmFunctionCompiler(Zone* zone, FunctionSig* sig,
   function_ = builder_->GetFunctionAt(index);
 }
 
+WasmFunctionCompiler::~WasmFunctionCompiler() {
+  if (trap_handler::UseTrapHandler() &&
+      !builder_->GetFunctionCode(function_index()).is_null()) {
+    const int handler_index = builder_->GetFunctionCode(function_index())
+                                  ->trap_handler_index()
+                                  ->value();
+    trap_handler::ReleaseHandlerData(handler_index);
+  }
+}
+
 FunctionSig* WasmRunnerBase::CreateSig(MachineType return_type,
                                        Vector<MachineType> param_types) {
   int return_count = return_type.IsNone() ? 0 : 1;

test/cctest/wasm/wasm-run-utils.h

@@ -259,6 +259,8 @@ class WasmFunctionWrapper : private compiler::GraphAndBuilders {
 // interpretation (by adding to the interpreter manually).
 class WasmFunctionCompiler : public compiler::GraphAndBuilders {
  public:
+  ~WasmFunctionCompiler();
+
   Isolate* isolate() { return builder_->isolate(); }
   CallDescriptor* descriptor() {
     if (descriptor_ == nullptr) {