Fuzzer: Add call-ref, call-ref-catch imports (WebAssembly#7137)

Similar to call-export*, these imports call a wasm function from outside the
module. The difference is that we send a function reference for them to call
(rather than an export index).

This gives more coverage, first by sending a ref from wasm to JS, and also
since we will now try to call anything that is sent. Exports, in comparison,
are filtered by the fuzzer to things that JS can handle, so this may lead to
more traps, but maybe also some new situations. This also leads to adding
more logic to execution-results.h to model JS trapping properly.

fuzz_shell.js is refactored to allow sharing code between call-export* and
call-ref*.

Author: kripken

scripts/fuzz_shell.js

@@ -160,6 +160,49 @@ function callFunc(func) {
   return func.apply(null, args);
 }
 
+// Calls a given function in a try-catch, swallowing JS exceptions, and return 1
+// if we did in fact swallow an exception. Wasm traps are not swallowed (see
+// details below).
+function tryCall(func) {
+  try {
+    func();
+    return 0;
+  } catch (e) {
+    // We only want to catch exceptions, not wasm traps: traps should still
+    // halt execution. Handling this requires different code in wasm2js, so
+    // check for that first (wasm2js does not define RuntimeError, so use
+    // that for the check - when wasm2js is run, we override the entire
+    // WebAssembly object with a polyfill, so we know exactly what it
+    // contains).
+    var wasm2js = !WebAssembly.RuntimeError;
+    if (!wasm2js) {
+      // When running native wasm, we can detect wasm traps.
+      if (e instanceof WebAssembly.RuntimeError) {
+        throw e;
+      }
+    }
+    var text = e + '';
+    // We must not swallow host limitations here: a host limitation is a
+    // problem that means we must not compare the outcome here to any other
+    // VM.
+    var hostIssues = ['requested new array is too large',
+                      'out of memory',
+                      'Maximum call stack size exceeded'];
+    if (wasm2js) {
+      // When wasm2js does trap, it just throws an "abort" error.
+      hostIssues.push('abort');
+    }
+    for (var hostIssue of hostIssues) {
+      if (text.includes(hostIssue)) {
+        throw e;
+      }
+    }
+    // Otherwise, this is a normal exception we want to catch (a wasm
+    // exception, or a conversion error on the wasm/JS boundary, etc.).
+    return 1;
+  }
+}
+
 // Table get/set operations need a BigInt if the table has 64-bit indexes. This
 // adds a proper cast as needed.
 function toAddressType(table, index) {
@@ -204,43 +247,15 @@ var imports = {
       callFunc(exportList[index].value);
     },
     'call-export-catch': (index) => {
-      try {
-        callFunc(exportList[index].value);
-        return 0;
-      } catch (e) {
-        // We only want to catch exceptions, not wasm traps: traps should still
-        // halt execution. Handling this requires different code in wasm2js, so
-        // check for that first (wasm2js does not define RuntimeError, so use
-        // that for the check - when wasm2js is run, we override the entire
-        // WebAssembly object with a polyfill, so we know exactly what it
-        // contains).
-        var wasm2js = !WebAssembly.RuntimeError;
-        if (!wasm2js) {
-          // When running native wasm, we can detect wasm traps.
-          if (e instanceof WebAssembly.RuntimeError) {
-            throw e;
-          }
-        }
-        var text = e + '';
-        // We must not swallow host limitations here: a host limitation is a
-        // problem that means we must not compare the outcome here to any other
-        // VM.
-        var hostIssues = ['requested new array is too large',
-                          'out of memory',
-                          'Maximum call stack size exceeded'];
-        if (wasm2js) {
-          // When wasm2js does trap, it just throws an "abort" error.
-          hostIssues.push('abort');
-        }
-        for (var hostIssue of hostIssues) {
-          if (text.includes(hostIssue)) {
-            throw e;
-          }
-        }
-        // Otherwise, this is a normal exception we want to catch (a wasm
-        // exception, or a conversion error on the wasm/JS boundary, etc.).
-        return 1;
-      }
+      return tryCall(() => callFunc(exportList[index].value));
+    },
+
+    // Funcref operations.
+    'call-ref': (ref) => {
+      callFunc(ref);
+    },
+    'call-ref-catch': (ref) => {
+      return tryCall(() => callFunc(ref));
     },
   },
   // Emscripten support.

src/tools/execution-results.h

@@ -105,13 +105,25 @@ struct LoggingExternalInterface : public ShellExternalInterface {
         tableStore(exportedTable, index, arguments[1]);
         return {};
       } else if (import->base == "call-export") {
-        callExport(arguments[0].geti32());
+        callExportAsJS(arguments[0].geti32());
         // Return nothing. If we wanted to return a value we'd need to have
         // multiple such functions, one for each signature.
         return {};
       } else if (import->base == "call-export-catch") {
         try {
-          callExport(arguments[0].geti32());
+          callExportAsJS(arguments[0].geti32());
+          return {Literal(int32_t(0))};
+        } catch (const WasmException& e) {
+          return {Literal(int32_t(1))};
+        }
+      } else if (import->base == "call-ref") {
+        callRefAsJS(arguments[0]);
+        // Return nothing. If we wanted to return a value we'd need to have
+        // multiple such functions, one for each signature.
+        return {};
+      } else if (import->base == "call-ref-catch") {
+        try {
+          callRefAsJS(arguments[0]);
           return {Literal(int32_t(0))};
         } catch (const WasmException& e) {
           return {Literal(int32_t(1))};
@@ -145,7 +157,7 @@ struct LoggingExternalInterface : public ShellExternalInterface {
     throwException(WasmException{Literal(payload)});
   }
 
-  Literals callExport(Index index) {
+  Literals callExportAsJS(Index index) {
     if (index >= wasm.exports.size()) {
       // No export.
       throwEmptyException();
@@ -155,20 +167,47 @@ struct LoggingExternalInterface : public ShellExternalInterface {
       // No callable export.
       throwEmptyException();
     }
-    auto* func = wasm.getFunction(exp->value);
+    return callFunctionAsJS(exp->value);
+  }
+
+  Literals callRefAsJS(Literal ref) {
+    if (!ref.isFunction()) {
+      // Not a callable ref.
+      throwEmptyException();
+    }
+    return callFunctionAsJS(ref.getFunc());
+  }
 
-    // TODO JS traps on some types on the boundary, which we should behave the
-    // same on. For now, this is not needed because the fuzzer will prune all
-    // non-JS-compatible exports anyhow.
+  // Call a function in a "JS-ey" manner, adding arguments as needed, and
+  // throwing if necessary, the same way JS does.
+  Literals callFunctionAsJS(Name name) {
+    auto* func = wasm.getFunction(name);
 
-    // Send default values as arguments, or trap if we need anything else.
+    // Send default values as arguments, or error if we need anything else.
     Literals arguments;
     for (const auto& param : func->getParams()) {
+      // An i64 param can work from JS, but fuzz_shell provides 0, which errors
+      // on attempts to convert it to BigInt. v128 cannot work at all.
+      if (param == Type::i64 || param == Type::v128) {
+        throwEmptyException();
+      }
       if (!param.isDefaultable()) {
         throwEmptyException();
       }
       arguments.push_back(Literal::makeZero(param));
     }
+
+    // Error on illegal results. Note that this happens, as per JS semantics,
+    // *before* the call.
+    for (const auto& result : func->getResults()) {
+      // An i64 result is fine: a BigInt will be provided. But v128 still
+      // errors.
+      if (result == Type::v128) {
+        throwEmptyException();
+      }
+    }
+
+    // Call the function.
     return instance->callFunction(func->name, arguments);
   }
 

src/tools/fuzzing.h

@@ -115,6 +115,8 @@ class TranslateToFuzzReader {
   Name tableSetImportName;
   Name callExportImportName;
   Name callExportCatchImportName;
+  Name callRefImportName;
+  Name callRefCatchImportName;
 
   std::unordered_map<Type, std::vector<Name>> globalsByType;
   std::unordered_map<Type, std::vector<Name>> mutableGlobalsByType;
@@ -244,7 +246,9 @@ class TranslateToFuzzReader {
   Expression* makeImportThrowing(Type type);
   Expression* makeImportTableGet();
   Expression* makeImportTableSet(Type type);
-  Expression* makeImportCallExport(Type type);
+  // Call either an export or a ref. We do this from a single function to better
+  // control the frequency of each.
+  Expression* makeImportCallCode(Type type);
   Expression* makeMemoryHashLogging();
 
   // Function creation

src/tools/fuzzing/fuzzing.cpp

@@ -771,22 +771,33 @@ void TranslateToFuzzReader::addImportLoggingSupport() {
 }
 
 void TranslateToFuzzReader::addImportCallingSupport() {
+  if (wasm.features.hasReferenceTypes() && closedWorld) {
+    // In closed world mode we must *remove* the call-ref* imports, if they
+    // exist in the initial content. These are not valid to call in closed-world
+    // mode as they call function references. (Another solution here would be to
+    // make closed-world issue validation errors on these imports, but that
+    // would require changes to the general-purpose validator.)
+    for (auto& func : wasm.functions) {
+      if (func->imported() && func->module == "fuzzing-support" &&
+          func->base.startsWith("call-ref")) {
+        // Make it non-imported, and with a simple body.
+        func->module = func->base = Name();
+        auto results = func->getResults();
+        func->body =
+          results.isConcrete() ? makeConst(results) : makeNop(Type::none);
+      }
+    }
+  }
+
   // Only add these some of the time, as they inhibit some fuzzing (things like
   // wasm-ctor-eval and wasm-merge are sensitive to the wasm being able to call
-  // its own exports, and to care about the indexes of the exports):
-  //
-  //  0 - none
-  //  1 - call-export
-  //  2 - call-export-catch
-  //  3 - call-export & call-export-catch
-  //  4 - none
-  //  5 - none
-  //
-  auto choice = upTo(6);
-  if (choice >= 4) {
+  // its own exports, and to care about the indexes of the exports).
+  if (oneIn(2)) {
     return;
   }
 
+  auto choice = upTo(16);
+
   if (choice & 1) {
     // Given an export index, call it from JS.
     callExportImportName = Names::getValidFunctionName(wasm, "call-export");
@@ -811,6 +822,34 @@ void TranslateToFuzzReader::addImportCallingSupport() {
     func->type = Signature(Type::i32, Type::i32);
     wasm.addFunction(std::move(func));
   }
+
+  // If the wasm will be used for closed-world testing, we cannot use the
+  // call-ref variants, as mentioned before.
+  if (wasm.features.hasReferenceTypes() && !closedWorld) {
+    if (choice & 4) {
+      // Given an funcref, call it from JS.
+      callRefImportName = Names::getValidFunctionName(wasm, "call-ref");
+      auto func = std::make_unique<Function>();
+      func->name = callRefImportName;
+      func->module = "fuzzing-support";
+      func->base = "call-ref";
+      func->type = Signature({Type(HeapType::func, Nullable)}, Type::none);
+      wasm.addFunction(std::move(func));
+    }
+
+    if (choice & 8) {
+      // Given an funcref, call it from JS and catch all exceptions (similar
+      // to callExportCatch), return 1 if we caught).
+      callRefCatchImportName =
+        Names::getValidFunctionName(wasm, "call-ref-catch");
+      auto func = std::make_unique<Function>();
+      func->name = callRefCatchImportName;
+      func->module = "fuzzing-support";
+      func->base = "call-ref-catch";
+      func->type = Signature(Type(HeapType::func, Nullable), Type::i32);
+      wasm.addFunction(std::move(func));
+    }
+  }
 }
 
 void TranslateToFuzzReader::addImportThrowingSupport() {
@@ -998,27 +1037,48 @@ Expression* TranslateToFuzzReader::makeImportTableSet(Type type) {
     Type::none);
 }
 
-Expression* TranslateToFuzzReader::makeImportCallExport(Type type) {
-  // The none-returning variant just does the call. The i32-returning one
-  // catches any errors and returns 1 when it saw an error. Based on the
-  // variant, pick which to call, and the maximum index to call.
-  Name target;
+Expression* TranslateToFuzzReader::makeImportCallCode(Type type) {
+  // Call code: either an export or a ref. Each has a catching and non-catching
+  // variant. The catching variants return i32, the others none.
+  assert(type == Type::none || type == Type::i32);
+  auto catching = type == Type::i32;
+  auto exportTarget =
+    catching ? callExportCatchImportName : callExportImportName;
+  auto refTarget = catching ? callRefCatchImportName : callRefImportName;
+
+  // We want to call a ref less often, as refs are more likely to error (a
+  // function reference can have arbitrary params and results, including things
+  // that error on the JS boundary; an export is already filtered for such
+  // things in some cases - when we legalize the boundary - and even if not, we
+  // emit lots of void(void) functions - all the invoke_foo functions - that are
+  // safe to call).
+  if (refTarget) {
+    // This matters a lot more in the variants that do *not* catch (in the
+    // catching ones, we just get a result of 1, but when not caught it halts
+    // execution).
+    if ((catching && (!exportTarget || oneIn(2))) || (!catching && oneIn(4))) {
+      // Most of the time make a non-nullable funcref, to avoid errors.
+      auto refType = Type(HeapType::func, oneIn(10) ? Nullable : NonNullable);
+      return builder.makeCall(refTarget, {make(refType)}, type);
+    }
+  }
+
+  if (!exportTarget) {
+    // We decided not to emit a call-ref here, due to fear of erroring, and
+    // there is no call-export, so just emit something trivial.
+    return makeTrivial(type);
+  }
+
+  // Pick the maximum export index to call.
   Index maxIndex = wasm.exports.size();
-  if (type == Type::none) {
-    target = callExportImportName;
-  } else if (type == Type::i32) {
-    target = callExportCatchImportName;
-    // This never traps, so we can be less careful, but we do still want to
-    // avoid trapping a lot as executing code is more interesting. (Note that
+  if (type == Type::i32) {
+    // This swallows errors, so we can be less careful, but we do still want to
+    // avoid swallowing a lot as executing code is more interesting. (Note that
     // even though we double here, the risk is not that great: we are still
     // adding functions as we go, so the first half of functions/exports can
     // double here and still end up in bounds by the time we've added them all.)
     maxIndex = (maxIndex + 1) * 2;
-  } else {
-    WASM_UNREACHABLE("bad import.call");
   }
-  // We must have set up the target function.
-  assert(target);
 
   // Most of the time, call a valid export index in the range we picked, but
   // sometimes allow anything at all.
@@ -1027,7 +1087,7 @@ Expression* TranslateToFuzzReader::makeImportCallExport(Type type) {
     index = builder.makeBinary(
       RemUInt32, index, builder.makeConst(int32_t(maxIndex)));
   }
-  return builder.makeCall(target, {index}, type);
+  return builder.makeCall(exportTarget, {index}, type);
 }
 
 Expression* TranslateToFuzzReader::makeMemoryHashLogging() {
@@ -1705,8 +1765,8 @@ Expression* TranslateToFuzzReader::_makeConcrete(Type type) {
     options.add(FeatureSet::Atomics, &Self::makeAtomic);
   }
   if (type == Type::i32) {
-    if (callExportCatchImportName) {
-      options.add(FeatureSet::MVP, &Self::makeImportCallExport);
+    if (callExportCatchImportName || callRefCatchImportName) {
+      options.add(FeatureSet::MVP, &Self::makeImportCallCode);
     }
     options.add(FeatureSet::ReferenceTypes, &Self::makeRefIsNull);
     options.add(FeatureSet::ReferenceTypes | FeatureSet::GC,
@@ -1787,8 +1847,8 @@ Expression* TranslateToFuzzReader::_makenone() {
   if (tableSetImportName) {
     options.add(FeatureSet::ReferenceTypes, &Self::makeImportTableSet);
   }
-  if (callExportImportName) {
-    options.add(FeatureSet::MVP, &Self::makeImportCallExport);
+  if (callExportImportName || callRefImportName) {
+    options.add(FeatureSet::MVP, &Self::makeImportCallCode);
   }
   return (this->*pick(options))(Type::none);
 }