[wasm-gc][bug] call_indirect should check for null table entries

This was not happening when there was no need to typecheck the entry.

Additional changes:
- Add tests with null table entries for typed and untyped function
  tables.
- Allow AddIndirectFunctionTable in wasm-run-utils to specify table
  type.
- Add possibility to define tables in test-gc.cc.
- Merge trapTableOutOfBounds with trapInvalidFunc.
- Use trapTableOutOfBounds in call_indirect as appropriate.
- Fix emission of table types in wasm-module-builder.cc.

Bug: v8:9495
Change-Id: I4a857ff4378e5a87dc0646d94b4c75635a43c55b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2442622Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70311}

src/builtins/base.tq

@@ -326,7 +326,6 @@ extern enum MessageTemplate {
   kWasmTrapDivUnrepresentable,
   kWasmTrapRemByZero,
   kWasmTrapFloatUnrepresentable,
-  kWasmTrapFuncInvalid,
   kWasmTrapFuncSigMismatch,
   kWasmTrapDataSegmentDropped,
   kWasmTrapElemSegmentDropped,

src/builtins/wasm.tq

@@ -385,10 +385,6 @@ builtin ThrowWasmTrapFloatUnrepresentable(): JSAny {
   tail WasmTrap(SmiConstant(MessageTemplate::kWasmTrapFloatUnrepresentable));
 }
 
-builtin ThrowWasmTrapFuncInvalid(): JSAny {
-  tail WasmTrap(SmiConstant(MessageTemplate::kWasmTrapFuncInvalid));
-}
-
 builtin ThrowWasmTrapFuncSigMismatch(): JSAny {
   tail WasmTrap(SmiConstant(MessageTemplate::kWasmTrapFuncSigMismatch));
 }

src/common/globals.h

@@ -1598,7 +1598,6 @@ enum class LoadSensitivity {
   V(TrapDivUnrepresentable)        \
   V(TrapRemByZero)                 \
   V(TrapFloatUnrepresentable)      \
-  V(TrapFuncInvalid)               \
   V(TrapFuncSigMismatch)           \
   V(TrapDataSegmentDropped)        \
   V(TrapElemSegmentDropped)        \

src/common/message-template.h

@@ -553,13 +553,12 @@ namespace internal {
   T(WasmTrapDivUnrepresentable, "divide result unrepresentable")               \
   T(WasmTrapRemByZero, "remainder by zero")                                    \
   T(WasmTrapFloatUnrepresentable, "float unrepresentable in integer range")    \
-  T(WasmTrapFuncInvalid, "invalid index into function table")                  \
+  T(WasmTrapTableOutOfBounds, "table index is out of bounds")                  \
   T(WasmTrapFuncSigMismatch, "function signature mismatch")                    \
   T(WasmTrapMultiReturnLengthMismatch, "multi-return length mismatch")         \
   T(WasmTrapTypeError, "wasm function signature contains illegal type")        \
   T(WasmTrapDataSegmentDropped, "data segment has been dropped")               \
   T(WasmTrapElemSegmentDropped, "element segment has been dropped")            \
-  T(WasmTrapTableOutOfBounds, "table access out of bounds")                    \
   T(WasmTrapBrOnExnNull, "br_on_exn on null value")                            \
   T(WasmTrapRethrowNull, "rethrowing null value")                              \
   T(WasmTrapNullDereference, "dereferencing a null pointer")                   \

src/compiler/common-operator.cc

@@ -569,7 +569,7 @@ IfValueParameters const& IfValueParametersOf(const Operator* op) {
   V(TrapDivUnrepresentable)        \
   V(TrapRemByZero)                 \
   V(TrapFloatUnrepresentable)      \
-  V(TrapFuncInvalid)               \
+  V(TrapTableOutOfBounds)          \
   V(TrapFuncSigMismatch)
 
 #define CACHED_PARAMETER_LIST(V) \

src/compiler/wasm-compiler.cc

@@ -2672,7 +2672,7 @@ Node* WasmGraphBuilder::BuildWasmCall(const wasm::FunctionSig* sig,
                                       wasm::WasmCodePosition position,
                                       Node* instance_node,
                                       UseRetpoline use_retpoline) {
-  auto call_descriptor =
+  CallDescriptor* call_descriptor =
       GetWasmCallDescriptor(mcgraph()->zone(), sig, use_retpoline);
   const Operator* op = mcgraph()->common()->Call(call_descriptor);
   Node* call = BuildCallNode(sig, args, position, instance_node, op);
@@ -2699,7 +2699,7 @@ Node* WasmGraphBuilder::BuildWasmReturnCall(const wasm::FunctionSig* sig,
                                             wasm::WasmCodePosition position,
                                             Node* instance_node,
                                             UseRetpoline use_retpoline) {
-  auto call_descriptor =
+  CallDescriptor* call_descriptor =
       GetWasmCallDescriptor(mcgraph()->zone(), sig, use_retpoline);
   const Operator* op = mcgraph()->common()->TailCall(call_descriptor);
   Node* call = BuildCallNode(sig, args, position, instance_node, op);
@@ -2878,7 +2878,7 @@ Node* WasmGraphBuilder::BuildIndirectCall(uint32_t table_index,
 
   // Bounds check against the table size.
   Node* in_bounds = graph()->NewNode(machine->Uint32LessThan(), key, ift_size);
-  TrapIfFalse(wasm::kTrapFuncInvalid, in_bounds, position);
+  TrapIfFalse(wasm::kTrapTableOutOfBounds, in_bounds, position);
 
   // Mask the key to prevent SSCA.
   if (untrusted_code_mitigations_) {
@@ -2896,20 +2896,27 @@ Node* WasmGraphBuilder::BuildIndirectCall(uint32_t table_index,
   Node* int32_scaled_key = Uint32ToUintptr(
       graph()->NewNode(machine->Word32Shl(), key, Int32Constant(2)));
 
+  Node* loaded_sig = SetEffect(
+      graph()->NewNode(machine->Load(MachineType::Int32()), ift_sig_ids,
+                       int32_scaled_key, effect(), control()));
   // Check that the dynamic type of the function is a subtype of its static
   // (table) type. Currently, the only subtyping between function types is
   // $t <: funcref for all $t: function_type.
   // TODO(7748): Expand this with function subtyping.
-  if (env_->module->tables[table_index].type == wasm::kWasmFuncRef) {
+  const bool needs_typechecking =
+      env_->module->tables[table_index].type == wasm::kWasmFuncRef;
+  if (needs_typechecking) {
     int32_t expected_sig_id = env_->module->signature_ids[sig_index];
-
-    Node* loaded_sig = SetEffect(
-        graph()->NewNode(machine->Load(MachineType::Int32()), ift_sig_ids,
-                         int32_scaled_key, effect(), control()));
-    Node* sig_match = graph()->NewNode(machine->WordEqual(), loaded_sig,
+    Node* sig_match = graph()->NewNode(machine->Word32Equal(), loaded_sig,
                                        Int32Constant(expected_sig_id));
-
     TrapIfFalse(wasm::kTrapFuncSigMismatch, sig_match, position);
+  } else {
+    // We still have to check that the entry is initialized.
+    // TODO(9495): Skip this check for non-nullable tables when they are
+    // allowed.
+    Node* function_is_null =
+        graph()->NewNode(machine->Word32Equal(), loaded_sig, Int32Constant(-1));
+    TrapIfTrue(wasm::kTrapNullDereference, function_is_null, position);
   }
 
   Node* tagged_scaled_key;

src/wasm/baseline/liftoff-compiler.cc

@@ -3916,7 +3916,7 @@ class LiftoffCompiler {
 
     // Bounds check against the table size.
     Label* invalid_func_label = AddOutOfLineTrap(
-        decoder->position(), WasmCode::kThrowWasmTrapFuncInvalid);
+        decoder->position(), WasmCode::kThrowWasmTrapTableOutOfBounds);
 
     uint32_t canonical_sig_num = env_->module->signature_ids[imm.sig_index];
     DCHECK_GE(canonical_sig_num, 0);

src/wasm/wasm-module-builder.cc

@@ -597,7 +597,7 @@ void WasmModuleBuilder::WriteTo(ZoneBuffer* buffer) const {
     size_t start = EmitSection(kTableSectionCode, buffer);
     buffer->write_size(tables_.size());
     for (const WasmTable& table : tables_) {
-      buffer->write_u8(table.type.value_type_code());
+      WriteValueType(buffer, table.type);
       buffer->write_u8(table.has_maximum ? kWithMaximum : kNoMaximum);
       buffer->write_size(table.min_size);
       if (table.has_maximum) buffer->write_size(table.max_size);

test/cctest/wasm/test-gc.cc

@@ -88,6 +88,10 @@ class WasmGCTester {
 
   byte DefineSignature(FunctionSig* sig) { return builder_.AddSignature(sig); }
 
+  byte DefineTable(ValueType type, uint32_t min_size, uint32_t max_size) {
+    return builder_.AddTable(type, min_size, max_size);
+  }
+
   void CompileModule() {
     ZoneBuffer buffer(&zone);
     builder_.WriteTo(&buffer);
@@ -1048,6 +1052,21 @@ TEST(GlobalInitReferencingGlobal) {
   tester.CheckResult(func, 42);
 }
 
+TEST(IndirectNullSetManually) {
+  WasmGCTester tester;
+  byte sig_index = tester.DefineSignature(tester.sigs.i_i());
+  tester.DefineTable(ValueType::Ref(sig_index, kNullable), 1, 1);
+  byte func_index = tester.DefineFunction(
+      tester.sigs.i_i(), {},
+      {WASM_TABLE_SET(0, WASM_I32V(0), WASM_REF_NULL(sig_index)),
+       WASM_CALL_INDIRECT(sig_index, WASM_I32V(0), WASM_GET_LOCAL(0)),
+       kExprEnd});
+
+  tester.CompileModule();
+
+  tester.CheckHasThrown(func_index, 42);
+}
+
 TEST(JsAccess) {
   WasmGCTester tester;
   const byte type_index = tester.DefineStruct({F(wasm::kWasmI32, true)});

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

@@ -3490,6 +3490,29 @@ WASM_EXEC_TEST(IfInsideUnreachable) {
   CHECK_EQ(17, r.Call());
 }
 
+WASM_EXEC_TEST(IndirectNull) {
+  WasmRunner<int32_t> r(execution_tier);
+  FunctionSig sig(1, 0, &kWasmI32);
+  byte sig_index = r.builder().AddSignature(&sig);
+  r.builder().AddIndirectFunctionTable(nullptr, 1);
+
+  BUILD(r, WASM_CALL_INDIRECT(sig_index, WASM_I32V(0)));
+
+  CHECK_TRAP(r.Call());
+}
+
+WASM_EXEC_TEST(IndirectNullTyped) {
+  WasmRunner<int32_t> r(execution_tier);
+  FunctionSig sig(1, 0, &kWasmI32);
+  byte sig_index = r.builder().AddSignature(&sig);
+  r.builder().AddIndirectFunctionTable(nullptr, 1,
+                                       ValueType::Ref(sig_index, kNullable));
+
+  BUILD(r, WASM_CALL_INDIRECT(sig_index, WASM_I32V(0)));
+
+  CHECK_TRAP(r.Call());
+}
+
 // This test targets binops in Liftoff.
 // Initialize a number of local variables to force them into different
 // registers, then perform a binary operation on two of the locals.

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

@@ -170,7 +170,8 @@ Handle<JSFunction> TestingModuleBuilder::WrapCode(uint32_t index) {
 }
 
 void TestingModuleBuilder::AddIndirectFunctionTable(
-    const uint16_t* function_indexes, uint32_t table_size) {
+    const uint16_t* function_indexes, uint32_t table_size,
+    ValueType table_type) {
   Handle<WasmInstanceObject> instance = instance_object();
   uint32_t table_index = static_cast<uint32_t>(test_module_->tables.size());
   test_module_->tables.emplace_back();
@@ -178,7 +179,7 @@ void TestingModuleBuilder::AddIndirectFunctionTable(
   table.initial_size = table_size;
   table.maximum_size = table_size;
   table.has_maximum_size = true;
-  table.type = kWasmFuncRef;
+  table.type = table_type;
 
   {
     // Allocate the indirect function table.

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

@@ -201,7 +201,8 @@ class TestingModuleBuilder {
   // If function_indexes is {nullptr}, the contents of the table will be
   // initialized with null functions.
   void AddIndirectFunctionTable(const uint16_t* function_indexes,
-                                uint32_t table_size);
+                                uint32_t table_size,
+                                ValueType table_type = kWasmFuncRef);
 
   uint32_t AddBytes(Vector<const byte> bytes);
 

test/common/wasm/wasm-interpreter.cc

@@ -3273,7 +3273,7 @@ class WasmInterpreterInternals {
               code = result.interpreter_code;
               continue;  // Do not bump pc.
             case CallResult::INVALID_FUNC:
-              return DoTrap(kTrapFuncInvalid, pc);
+              return DoTrap(kTrapTableOutOfBounds, pc);
             case CallResult::SIGNATURE_MISMATCH:
               return DoTrap(kTrapFuncSigMismatch, pc);
           }
@@ -3319,7 +3319,7 @@ class WasmInterpreterInternals {
               continue;  // Do not bump pc.
             }
             case CallResult::INVALID_FUNC:
-              return DoTrap(kTrapFuncInvalid, pc);
+              return DoTrap(kTrapTableOutOfBounds, pc);
             case CallResult::SIGNATURE_MISMATCH:
               return DoTrap(kTrapFuncSigMismatch, pc);
           }

test/mjsunit/wasm/indirect-call-non-zero-table.js

@@ -79,11 +79,11 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
   assertEquals(v1, instance.exports.call1(0));
   assertEquals(v2, instance.exports.call1(1));
   assertEquals(v3, instance.exports.call1(2));
-  assertTraps(kTrapFuncInvalid, () => instance.exports.call1(3));
+  assertTraps(kTrapTableOutOfBounds, () => instance.exports.call1(3));
   assertEquals(v1, instance.exports.return_call1(0));
   assertEquals(v2, instance.exports.return_call1(1));
   assertEquals(v3, instance.exports.return_call1(2));
-  assertTraps(kTrapFuncInvalid, () => instance.exports.return_call1(3));
+  assertTraps(kTrapTableOutOfBounds, () => instance.exports.return_call1(3));
 
   // Try to call through the uninitialized table entry.
   assertTraps(kTrapFuncSigMismatch, () => instance.exports.call2(0));

test/mjsunit/wasm/indirect-calls.js

@@ -55,7 +55,7 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
   print(" --z1--");
   assertTraps(kTrapFuncSigMismatch, () => module.exports.main(2, 12, 33));
   print(" --w1--");
-  assertTraps(kTrapFuncInvalid, () => module.exports.main(3, 12, 33));
+  assertTraps(kTrapTableOutOfBounds, () => module.exports.main(3, 12, 33));
 })();
 
 (function Test2() {
@@ -99,7 +99,7 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
   print(" --q2--");
   assertTraps(kTrapFuncSigMismatch, () => module.exports.main(3, 12, 33));
   print(" --t2--");
-  assertTraps(kTrapFuncInvalid, () => module.exports.main(4, 12, 33));
+  assertTraps(kTrapTableOutOfBounds, () => module.exports.main(4, 12, 33));
 })();
 
 
@@ -151,7 +151,7 @@ function AddFunctions(builder) {
   assertEquals(35, module.exports.main(2, 1));
   assertEquals(32, module.exports.main(1, 2));
   assertEquals(31, module.exports.main(2, 2));
-  assertTraps(kTrapFuncInvalid, () => module.exports.main(12, 3));
+  assertTraps(kTrapTableOutOfBounds, () => module.exports.main(12, 3));
 })();
 
 (function ConstBaseTest() {
@@ -187,7 +187,7 @@ function AddFunctions(builder) {
     assertEquals(31, main(2, i + 1));
     assertEquals(33, main(1, i + 2));
     assertEquals(66, main(2, i + 2));
-    assertTraps(kTrapFuncInvalid, () => main(12, 10));
+    assertTraps(kTrapTableOutOfBounds, () => main(12, 10));
   }
 })();
 
@@ -224,6 +224,6 @@ function AddFunctions(builder) {
     assertEquals(35, main(2, i + 1));
     assertEquals(32, main(1, i + 2));
     assertEquals(31, main(2, i + 2));
-    assertTraps(kTrapFuncInvalid, () => main(12, 10));
+    assertTraps(kTrapTableOutOfBounds, () => main(12, 10));
   }
 })();

test/mjsunit/wasm/indirect-tables.js

@@ -333,8 +333,8 @@ function js_div(a, b) { return (a / b) | 0; }
 
   assertTraps(kTrapFuncSigMismatch, () => i1.exports.main(2));
   assertTraps(kTrapFuncSigMismatch, () => i2.exports.main(2));
-  assertTraps(kTrapFuncInvalid, () => i1.exports.main(3));
-  assertTraps(kTrapFuncInvalid, () => i2.exports.main(3));
+  assertTraps(kTrapTableOutOfBounds, () => i1.exports.main(3));
+  assertTraps(kTrapTableOutOfBounds, () => i2.exports.main(3));
 })();
 
 (function MismatchedTableSize() {

test/mjsunit/wasm/table-grow-from-wasm.js

@@ -93,7 +93,7 @@ function testGrowInternalAnyFuncTable(table_index) {
   assertTraps(kTrapFuncSigMismatch, () => instance.exports.call(size - 2));
   function growAndCheck(element, grow_by) {
     assertEquals(size, instance.exports.size());
-    assertTraps(kTrapFuncInvalid, () => instance.exports.call(size));
+    assertTraps(kTrapTableOutOfBounds, () => instance.exports.call(size));
     assertEquals(size, instance.exports.grow(dummy_func(element), grow_by));
     for (let i = 0; i < grow_by; ++i) {
       assertEquals(element, instance.exports.call(size + i));

test/mjsunit/wasm/table-grow.js

@@ -289,7 +289,7 @@ let id = (() => {  // identity exported function
       assertInvalidFunction = function(s) {
         assertThrows(
             () => instances[i].exports.main(s), WebAssembly.RuntimeError,
-            kTrapMsgs[kTrapFuncInvalid]);
+            kTrapMsgs[kTrapTableOutOfBounds]);
       }
       assertInvalidFunction(size);
       assertInvalidFunction(size + 1);

test/mjsunit/wasm/trap-location.js

@@ -32,7 +32,7 @@ function testTrapLocations(instance, expected_stack_length) {
   testWasmTrap(0, kTrapDivByZero, 14);
   testWasmTrap(1, kTrapMemOutOfBounds, 15);
   testWasmTrap(2, kTrapUnreachable, 28);
-  testWasmTrap(3, kTrapFuncInvalid, 32);
+  testWasmTrap(3, kTrapTableOutOfBounds, 32);
 }
 
 var builder = new WasmModuleBuilder();

test/mjsunit/wasm/wasm-module-builder.js

@@ -678,15 +678,14 @@ let kTrapDivByZero            = 2;
 let kTrapDivUnrepresentable   = 3;
 let kTrapRemByZero            = 4;
 let kTrapFloatUnrepresentable = 5;
-let kTrapFuncInvalid          = 6;
+let kTrapTableOutOfBounds     = 6;
 let kTrapFuncSigMismatch      = 7;
 let kTrapTypeError            = 8;
 let kTrapUnalignedAccess      = 9;
 let kTrapDataSegmentDropped   = 10;
 let kTrapElemSegmentDropped   = 11;
-let kTrapTableOutOfBounds     = 12;
-let kTrapBrOnExnNull          = 13;
-let kTrapRethrowNull          = 14;
+let kTrapBrOnExnNull          = 12;
+let kTrapRethrowNull          = 13;
 
 let kTrapMsgs = [
   "unreachable",
@@ -695,13 +694,12 @@ let kTrapMsgs = [
   "divide result unrepresentable",
   "remainder by zero",
   "float unrepresentable in integer range",
-  "invalid index into function table",
+  "table index is out of bounds",
   "function signature mismatch",
   "wasm function signature contains illegal type",
   "operation does not support unaligned accesses",
   "data segment has been dropped",
   "element segment has been dropped",
-  "table access out of bounds",
   "br_on_exn on null value",
   "rethrowing null value"
 ];