From 8a92ab5f5f0529fec4b71223858fac853837884e Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 18 Aug 2022 09:11:10 -0700
Subject: Validator: Validate unreachable calls more carefully (#4909)

Normally the validator will find stale types properly, by just running refinalize and seeing
if the type has changed (if so, then some code forgot to refinalize). However, refinalize
is a local operation, so it does not apply to calls: a call's proper type is determined by
the global information of the function we are calling. As a result, we would not notice
errors like this:

(call $foo) ;; type: unreachable

Refinalizing that would not change the type from unreachable to the proper type, since
that is global information.

To validate this properly, validate that a call whose type is unreachable actually has
an unreachable child. That rules out an invalid unreachable type here, which leaves
concrete types, that we already have proper global validation for. The code here is
generalized to handle non-call things as well, but it only helps expressions requiring
global validation, so it likely only helps global.get and a few others.
---
 src/wasm/wasm-validator.cpp | 57 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

(limited to 'src/wasm/wasm-validator.cpp')

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index e837c2dd6..f78a2301d 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -327,6 +327,63 @@ public:
         self->pushTask(visitPoppyExpression, currp);
       }
     }
+
+    // Also verify that only allowed expressions end up in the situation where
+    // the expression has type unreachable but there is no unreachable child.
+    // For example a Call with no unreachable child cannot be unreachable, but a
+    // Break can be.
+    if (curr->type == Type::unreachable) {
+      switch (curr->_id) {
+        case Expression::BreakId: {
+          // If there is a condition, that is already validated fully in
+          // visitBreak(). If there isn't a condition, then this is allowed to
+          // be unreachable even without an unreachable child. Either way, we
+          // can leave.
+          return;
+        }
+        case Expression::SwitchId:
+        case Expression::ReturnId:
+        case Expression::UnreachableId:
+        case Expression::ThrowId:
+        case Expression::RethrowId: {
+          // These can all be unreachable without an unreachable child.
+          return;
+        }
+        case Expression::CallId: {
+          if (curr->cast<Call>()->isReturn) {
+            return;
+          }
+          break;
+        }
+        case Expression::CallIndirectId: {
+          if (curr->cast<CallIndirect>()->isReturn) {
+            return;
+          }
+          break;
+        }
+        case Expression::CallRefId: {
+          if (curr->cast<CallRef>()->isReturn) {
+            return;
+          }
+          break;
+        }
+        default: {
+          break;
+        }
+      }
+
+      // If we reach here, then we must have an unreachable child.
+      bool hasUnreachableChild = false;
+      for (auto* child : ChildIterator(curr)) {
+        if (child->type == Type::unreachable) {
+          hasUnreachableChild = true;
+          break;
+        }
+      }
+      self->shouldBeTrue(hasUnreachableChild,
+                         curr,
+                         "unreachable instruction must have unreachable child");
+    }
   }
 
   void noteBreak(Name name, Expression* value, Expression* curr);
-- 
cgit v1.2.3