Fix bad param/var type error handling (WebAssembly#1499)

kripken · web-flow · commit 9b5ce4719920 · 2018-04-10T17:17:16.000-07:00
Improve error handling, validation, and assertions for having a non-concrete type in an inappropriate place. Fixes a fuzz testcase.
diff --git a/src/wasm-binary.h b/src/wasm-binary.h
@@ -840,6 +840,7 @@ class WasmBinaryBuilder {
   int32_t getS32LEB();
   int64_t getS64LEB();
   Type getType();
+  Type getConcreteType();
   Name getString();
   Name getInlineString();
   void verifyInt8(int8_t x);
diff --git a/src/wasm-builder.h b/src/wasm-builder.h
@@ -375,6 +375,7 @@ class Builder {
 
   static Index addVar(Function* func, Name name, Type type) {
     // always ok to add a var, it does not affect other indices
+    assert(isConcreteType(type));
     Index index = func->getNumLocals();
     if (name.is()) {
       func->localIndices[name] = index;
diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
@@ -1491,6 +1491,14 @@ Type WasmBinaryBuilder::getType() {
   }
 }
 
+Type WasmBinaryBuilder::getConcreteType() {
+  auto type = getType();
+  if (!isConcreteType(type)) {
+    throw ParseException("non-concrete type when one expected");
+  }
+  return type;
+}
+
 Name WasmBinaryBuilder::getString() {
   if (debug) std::cerr << "<==" << std::endl;
   size_t offset = getInt32();
@@ -1579,7 +1587,7 @@ void WasmBinaryBuilder::readSignatures() {
     size_t numParams = getU32LEB();
     if (debug) std::cerr << "num params: " << numParams << std::endl;
     for (size_t j = 0; j < numParams; j++) {
-      curr->params.push_back(getType());
+      curr->params.push_back(getConcreteType());
     }
     auto numResults = getU32LEB();
     if (numResults == 0) {
@@ -1667,7 +1675,7 @@ void WasmBinaryBuilder::readImports() {
       }
       case ExternalKind::Global: {
         curr->name = Name(std::string("gimport$") + std::to_string(i));
-        curr->globalType = getType();
+        curr->globalType = getConcreteType();
         auto globalMutable = getU32LEB();
         // TODO: actually use the globalMutable flag. Currently mutable global
         // imports is a future feature, to be implemented with thread support.
@@ -1734,7 +1742,7 @@ void WasmBinaryBuilder::readFunctions() {
     size_t numLocalTypes = getU32LEB();
     for (size_t t = 0; t < numLocalTypes; t++) {
       auto num = getU32LEB();
-      auto type = getType();
+      auto type = getConcreteType();
       while (num > 0) {
         vars.emplace_back(addVar(), type);
         num--;
@@ -1943,7 +1951,7 @@ void WasmBinaryBuilder::readGlobals() {
   if (debug) std::cerr << "num: " << num << std::endl;
   for (size_t i = 0; i < num; i++) {
     if (debug) std::cerr << "read one" << std::endl;
-    auto type = getType();
+    auto type = getConcreteType();
     auto mutable_ = getU32LEB();
     if (bool(mutable_) != mutable_) throw ParseException("Global mutability must be 0 or 1");
     auto* init = readExpression();
@@ -2057,11 +2065,16 @@ Expression* WasmBinaryBuilder::popNonVoidExpression() {
     block->list.push_back(expressions.back());
     expressions.pop_back();
   }
-  auto type = block->list[0]->type;
   requireFunctionContext("popping void where we need a new local");
-  auto local = builder.addVar(currFunction, type);
-  block->list[0] = builder.makeSetLocal(local, block->list[0]);
-  block->list.push_back(builder.makeGetLocal(local, type));
+  auto type = block->list[0]->type;
+  if (isConcreteType(type)) {
+    auto local = builder.addVar(currFunction, type);
+    block->list[0] = builder.makeSetLocal(local, block->list[0]);
+    block->list.push_back(builder.makeGetLocal(local, type));
+  } else {
+    assert(type == unreachable);
+    // nothing to do here - unreachable anyhow
+  }
   block->finalize();
   return block;
 }
diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
@@ -795,6 +795,12 @@ void FunctionValidator::visitHost(Host* curr) {
 }
 
 void FunctionValidator::visitFunction(Function* curr) {
+  for (auto type : curr->params) {
+    shouldBeTrue(isConcreteType(type), curr, "params must be concretely typed");
+  }
+  for (auto type : curr->vars) {
+    shouldBeTrue(isConcreteType(type), curr, "vars must be concretely typed");
+  }
   // if function has no result, it is ignored
   // if body is unreachable, it might be e.g. a return
   if (curr->body->type != unreachable) {
diff --git a/test/badvartype.wasm b/test/badvartype.wasm
diff --git a/test/badvartype.wasm.fromBinary b/test/badvartype.wasm.fromBinary
