[wasm][bug] Fix a couple of bugs in validation of unreachable code

Changes:
- SetBlockType now instantiates the block's start merge with values of
  the correct type in unreachable code.
- EnsureStackArguments now keeps the existing stack values and moves
  them over the new bottom values.
- Drop stack size validation in Drop().
- Add new tests in unreachable-validation.js.

Change-Id: Ie68b3d9abb0a41d1623d4a123fb526e71941c4e7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2902733
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74650}

src/wasm/function-body-decoder-impl.h

@@ -3573,13 +3573,18 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     InitMerge(&c->end_merge, imm.out_arity(), [pc, &imm](uint32_t i) {
       return Value{pc, imm.out_type(i)};
     });
-    InitMerge(&c->start_merge, imm.in_arity(),
-              [args](uint32_t i) { return args[i]; });
+    InitMerge(&c->start_merge, imm.in_arity(), [pc, &imm, args](uint32_t i) {
+      // The merge needs to be instantiated with Values of the correct type even
+      // in the presence of bottom values (i.e. in unreachable code). Since
+      // bottom Values will never be used for code generation, we can safely
+      // instantiate new ones in that case.
+      return args[i].type != kWasmBottom ? args[i] : Value{pc, imm.in_type(i)};
+    });
   }
 
   V8_INLINE void EnsureStackArguments(int count) {
     uint32_t limit = control_.back().stack_depth;
-    if (stack_size() >= count + limit) return;
+    if (V8_LIKELY(stack_size() >= count + limit)) return;
     EnsureStackArguments_Slow(count, limit);
   }
 
@@ -3588,15 +3593,21 @@ class WasmFullDecoder : public WasmDecoder<validate> {
       int index = count - stack_size() - 1;
       NotEnoughArgumentsError(index);
     }
-    // Silently create unreachable values out of thin air. Since we push them
-    // onto the stack, while conceptually we should be inserting them under
-    // any existing elements, we have to avoid validation failures that would
-    // be caused by finding non-unreachable values in the wrong slot, so we
-    // replace the entire current scope's values.
-    Drop(static_cast<int>(stack_size() - limit));
-    EnsureStackSpace(count + limit - stack_size());
-    while (stack_size() < count + limit) {
-      Push(UnreachableValue(this->pc_));
+    // Silently create unreachable values out of thin air underneath the
+    // existing stack values. To do so, we have to move existing stack values
+    // upwards in the stack, then instantiate the new Values as
+    // {UnreachableValue}.
+    int current_values = stack_size() - limit;
+    int additional_values = count - current_values;
+    DCHECK_GT(additional_values, 0);
+    EnsureStackSpace(additional_values);
+    stack_end_ += additional_values;
+    Value* stack_base = stack_value(current_values + additional_values);
+    for (int i = current_values - 1; i >= 0; i--) {
+      stack_base[additional_values + i] = stack_base[i];
+    }
+    for (int i = 0; i < additional_values; i++) {
+      stack_base[i] = UnreachableValue(this->pc_);
     }
   }
 
@@ -3611,6 +3622,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     }
     return args;
   }
+  // Drops a number of stack elements equal to the {sig}'s parameter count (0 if
+  // {sig} is null), or all of them if less are present.
   V8_INLINE void DropArgs(const FunctionSig* sig) {
     int count = sig ? static_cast<int>(sig->parameter_count()) : 0;
     Drop(count);
@@ -3626,6 +3639,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     }
     return args;
   }
+  // Drops a number of stack elements equal to the struct's field count, or all
+  // of them if less are present.
   V8_INLINE void DropArgs(const StructType* type) {
     Drop(static_cast<int>(type->field_count()));
   }
@@ -4759,22 +4774,20 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     }
   }
 
+  // Drop the top {count} stack elements, or all of them if less than {count}
+  // are present.
   V8_INLINE void Drop(int count = 1) {
     DCHECK(!control_.empty());
     uint32_t limit = control_.back().stack_depth;
-    // TODO(wasm): This check is often redundant.
     if (V8_UNLIKELY(stack_size() < limit + count)) {
-      // Popping past the current control start in reachable code.
-      if (!VALIDATE(!current_code_reachable_and_ok_)) {
-        NotEnoughArgumentsError(0);
-      }
       // Pop what we can.
       count = std::min(count, static_cast<int>(stack_size() - limit));
     }
     DCHECK_LE(stack_, stack_end_ - count);
     stack_end_ -= count;
   }
-  // For more descriptive call sites:
+  // Drop the top stack element if present. Takes a Value input for more
+  // descriptive call sites.
   V8_INLINE void Drop(const Value& /* unused */) { Drop(1); }
 
   enum StackElementsCountMode : bool {
@@ -4807,7 +4820,9 @@ class WasmFullDecoder : public WasmDecoder<validate> {
             : merge_type == kReturnMerge ? "return" : "fallthru";
     uint32_t arity = merge->arity;
     uint32_t actual = stack_size() - control_.back().stack_depth;
-    if (V8_LIKELY(current_code_reachable_and_ok_)) {
+    // Here we have to check for !unreachable(), because we need to typecheck as
+    // if the current code is reachable even if it is spec-only reachable.
+    if (V8_LIKELY(!control_.back().unreachable())) {
       if (V8_UNLIKELY(strict_count ? actual != drop_values + arity
                                    : actual < drop_values + arity)) {
         this->DecodeError("expected %u elements on the stack for %s, found %u",

test/mjsunit/wasm/unreachable-validation.js

@@ -11,6 +11,10 @@ let unittest = true;
 
 function run(expected, name, code) {
   let builder = new WasmModuleBuilder();
+  // Index 0
+  builder.addType(kSig_i_i);
+  // Index 1
+  builder.addType(kSig_i_ii);
   builder.addFunction("main", kSig_v_v).
     addBody(code);
   let buffer = builder.toBuffer();
@@ -36,6 +40,8 @@ let X = undefined;
 
 let nop = kExprNop;
 let iadd = kExprI32Add;
+let ieqz = kExprI32Eqz;
+let leqz = kExprI64Eqz;
 let unr = kExprUnreachable;
 let ret = kExprReturn;
 let br0 = [kExprBr, 0];
@@ -45,6 +51,7 @@ let brt1 = [kExprBrTable, 0, 1];
 let brt01 = [kExprBrTable, 1, 0, 1];
 let f32 = [kExprF32Const, 0, 0, 0, 0];
 let zero = [kExprI32Const, 0];
+let zero64 = [kExprI64Const, 0];
 let if_else_empty = [kExprIf, kWasmVoid, kExprElse, kExprEnd];
 let if_unr = [kExprIf, kWasmVoid, kExprUnreachable, kExprEnd];
 let if_else_unr = [kExprIf, kWasmVoid, kExprUnreachable, kExprElse, kExprUnreachable, kExprEnd];
@@ -53,6 +60,8 @@ let loop_unr = [kExprLoop, kWasmVoid, kExprUnreachable, kExprEnd];
 let block_block_unr = [kExprBlock, kWasmVoid, kExprBlock, kWasmVoid, kExprUnreachable, kExprEnd, kExprEnd];
 let block = [kExprBlock, kWasmVoid]
 let iblock = [kExprBlock, kWasmI32]
+let i_iblock = [kExprBlock, 0]
+let i_iiblock = [kExprBlock, 1]
 let fblock = [kExprBlock, kWasmF32]
 let end = kExprEnd;
 let drop = kExprDrop;
@@ -92,6 +101,10 @@ run(I, '(block U) iadd drop', [...block_unr, iadd, drop]);
 run(I, '(block (block U)) iadd drop', [...block_block_unr, iadd, drop]);
 run(I, '(loop U) iadd drop', [...loop_unr, iadd]);
 run(I, '(if 0 U U) iadd drop', [...zero, ...if_else_unr, iadd, drop]);
+run(I, 'U (i_iblock leqz)', [unr, ...i_iblock, leqz, end, drop]);
+run(V, 'U (i_iblock ieqz)', [unr, ...i_iblock, ieqz, end, drop]);
+run(I, 'U (iblock iadd)', [unr, ...iblock, iadd, end, drop]);
+run(I, 'U zero64 (i_iiblock iadd) drop', [unr, ...zero64, ...i_iiblock, iadd, end, drop])
 
 run(V, 'U 0 0 iadd drop', [unr, ...zero, ...zero, iadd, drop]);
 run(V, "(block U) 0 0 iadd drop", [...block_unr, ...zero, ...zero, iadd, drop]);