[Custom Descriptors] Validate features in TypeBuilder (#7790)

The Binaryen validator does not validate all types because it does not
want to pay the cost of walking the module to gather the types.
Furthermore, even if it did walk the module to gather types, it would
still have no way of validating completely unused types. However,
correctly validating that a module's types use only the allowed features
can be important, for example to prevent the fuzzer from using initial
contents that use a feature with a handler that cannot handle that
feature. To achieve this validation without paying the cost in the
validator, validate features used by types in TypeBuilder instead. For
now the only feature validated is custom descriptors.

To avoid introducing new errors, ensure that all features are available
when parsing in wasm-dis and wasm-reduce. Also tweak error messages to
make the binary and text parsers consistent so we can simplify the test.

Author: tlively

src/parser/parse-2-typedefs.cpp

@@ -24,7 +24,7 @@ Result<> parseTypeDefs(
   IndexMap& typeIndices,
   std::vector<HeapType>& types,
   std::unordered_map<HeapType, std::unordered_map<Name, Index>>& typeNames) {
-  TypeBuilder builder(decls.typeDefs.size());
+  TypeBuilder builder(decls.typeDefs.size(), decls.wasm.features);
   ParseTypeDefsCtx ctx(input, builder, typeIndices);
   for (auto& recType : decls.recTypeDefs) {
     WithPosition with(ctx, recType.pos);

src/parser/wast-parser.cpp

@@ -16,6 +16,7 @@
 
 #include "lexer.h"
 #include "literal.h"
+#include "wasm-features.h"
 #include "wat-parser.h"
 
 namespace wasm::WATParser {
@@ -123,6 +124,7 @@ Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
     // start and parse it normally.
     in = std::move(reset);
     auto wasm = std::make_shared<Module>();
+    wasm->features = FeatureSet::All;
     CHECK_ERR(parseModule(*wasm, in));
     return wasm;
   }
@@ -479,6 +481,7 @@ Result<WASTScript> wast(Lexer& in) {
       // The entire script might be a single module comprising a sequence of
       // module fields with a top-level `(module ...)`.
       auto wasm = std::make_shared<Module>();
+      wasm->features = FeatureSet::All;
       auto parsed = parseModule(*wasm, in.buffer);
       if (parsed.getErr()) {
         // No, that wasn't the problem. Return the original error.

src/tools/wasm-dis.cpp

@@ -21,6 +21,7 @@
 #include "source-map.h"
 #include "support/colors.h"
 #include "support/file.h"
+#include "wasm-features.h"
 #include "wasm-io.h"
 
 #include "tool-options.h"
@@ -66,6 +67,11 @@ int main(int argc, const char* argv[]) {
   }
   Module wasm;
   options.applyOptionsBeforeParse(wasm);
+  // Temporarily apply all features during parsing to avoid any errors. See note
+  // below on skipping validation.
+  auto enabledFeatures = wasm.features;
+  wasm.features = FeatureSet::All;
+
   try {
     ModuleReader().readBinary(options.extra["infile"], wasm, sourceMapFilename);
   } catch (ParseException& p) {
@@ -91,6 +97,7 @@ int main(int argc, const char* argv[]) {
   //       better to have an "autodetect" code path that enables used features
   //       eventually.
 
+  wasm.features = enabledFeatures;
   if (options.debug) {
     std::cerr << "Printing..." << std::endl;
   }

src/tools/wasm-reduce.cpp

@@ -379,6 +379,9 @@ struct Reducer
 
     toolOptions.applyOptionsBeforeParse(*module);
 
+    // Assume we may need all features.
+    module->features = FeatureSet::All;
+
     ModuleReader reader;
     try {
       reader.read(working, *module);
@@ -390,12 +393,6 @@ struct Reducer
 
     toolOptions.applyOptionsAfterParse(*module);
 
-    // If there is no features section, assume we may need them all (without
-    // this, a module with no features section but that uses e.g. atomics and
-    // bulk memory would not work).
-    if (!module->hasFeaturesSection) {
-      module->features = FeatureSet::All;
-    }
     builder = std::make_unique<Builder>(*module);
     setModule(module.get());
   }

src/wasm-binary.h

@@ -1705,7 +1705,7 @@ class WasmBinaryReader {
 
   static Name escape(Name name);
   void readNames(size_t sectionPos, size_t payloadLen);
-  void readFeatures(size_t payloadLen);
+  void readFeatures(size_t sectionPos, size_t payloadLen);
   void readDylink(size_t payloadLen);
   void readDylink0(size_t payloadLen);
 

src/wasm-type.h

@@ -717,7 +717,7 @@ struct TypeBuilder {
   struct Impl;
   std::unique_ptr<Impl> impl;
 
-  TypeBuilder(size_t n);
+  TypeBuilder(size_t n, FeatureSet features = FeatureSet::All);
   TypeBuilder() : TypeBuilder(0) {}
   ~TypeBuilder();
 
@@ -862,6 +862,8 @@ struct TypeBuilder {
     InvalidUnsharedDescriptor,
     // A non-shared type described by a shared type.
     InvalidUnsharedDescribes,
+    // The custom descriptors feature is missing.
+    RequiresCustomDescriptors,
   };
 
   struct Error {

src/wasm/wasm-binary.cpp

@@ -2009,6 +2009,8 @@ void WasmBinaryReader::preScan() {
       } else if (debugInfo &&
                  sectionName == BinaryConsts::CustomSections::Name) {
         readNames(oldPos, payloadLen);
+      } else if (sectionName == BinaryConsts::CustomSections::TargetFeatures) {
+        readFeatures(oldPos, payloadLen);
       }
       // TODO: We could stop early in some cases, if we've seen enough (e.g.
       //       seeing Code implies no BranchHint will appear, due to ordering).
@@ -2139,11 +2141,11 @@ void WasmBinaryReader::readCustomSection(size_t payloadLen) {
     throwError("bad user section size");
   }
   payloadLen -= read;
-  if (sectionName.equals(BinaryConsts::CustomSections::Name)) {
-    // We already read the name section before anything else.
+  if (sectionName.equals(BinaryConsts::CustomSections::Name) ||
+      sectionName.equals(BinaryConsts::CustomSections::TargetFeatures)) {
+    // We already read the name and target features sections before anything
+    // else.
     pos += payloadLen;
-  } else if (sectionName.equals(BinaryConsts::CustomSections::TargetFeatures)) {
-    readFeatures(payloadLen);
   } else if (sectionName.equals(BinaryConsts::CustomSections::Dylink)) {
     readDylink(payloadLen);
   } else if (sectionName.equals(BinaryConsts::CustomSections::Dylink0)) {
@@ -2550,7 +2552,7 @@ void WasmBinaryReader::readMemories() {
 }
 
 void WasmBinaryReader::readTypes() {
-  TypeBuilder builder(getU32LEB());
+  TypeBuilder builder(getU32LEB(), wasm.features);
 
   auto readHeapType = [&]() -> std::pair<HeapType, Exactness> {
     int64_t htCode = getS64LEB(); // TODO: Actually s33
@@ -2738,7 +2740,7 @@ void WasmBinaryReader::readTypes() {
 
   auto result = builder.build();
   if (auto* err = result.getError()) {
-    Fatal() << "Invalid type: " << err->reason << " at index " << err->index;
+    Fatal() << "invalid type: " << err->reason << " at index " << err->index;
   }
   types = std::move(*result);
 
@@ -5180,10 +5182,9 @@ void WasmBinaryReader::readNames(size_t sectionPos, size_t payloadLen) {
   }
 }
 
-void WasmBinaryReader::readFeatures(size_t payloadLen) {
+void WasmBinaryReader::readFeatures(size_t sectionPos, size_t payloadLen) {
   wasm.hasFeaturesSection = true;
 
-  auto sectionPos = pos;
   size_t numFeatures = getU32LEB();
   for (size_t i = 0; i < numFeatures; ++i) {
     uint8_t prefix = getInt8();

src/wasm/wasm-type.cpp

@@ -1456,6 +1456,8 @@ std::ostream& operator<<(std::ostream& os, TypeBuilder::ErrorReason reason) {
       return os << "Heap type has an invalid unshared descriptor";
     case TypeBuilder::ErrorReason::InvalidUnsharedDescribes:
       return os << "Heap type describes an invalid unshared type";
+    case TypeBuilder::ErrorReason::RequiresCustomDescriptors:
+      return os << "custom descriptors required but not enabled";
   }
   WASM_UNREACHABLE("Unexpected error reason");
 }
@@ -2251,11 +2253,14 @@ struct TypeBuilder::Impl {
 
   std::vector<Entry> entries;
 
-  Impl(size_t n) : entries(n) {}
+  // We will validate features as we go.
+  FeatureSet features;
+
+  Impl(size_t n, FeatureSet features) : entries(n), features(features) {}
 };
 
-TypeBuilder::TypeBuilder(size_t n) {
-  impl = std::make_unique<TypeBuilder::Impl>(n);
+TypeBuilder::TypeBuilder(size_t n, FeatureSet features) {
+  impl = std::make_unique<TypeBuilder::Impl>(n, features);
 }
 
 TypeBuilder::~TypeBuilder() = default;
@@ -2403,7 +2408,9 @@ bool isValidSupertype(const HeapTypeInfo& sub, const HeapTypeInfo& super) {
 }
 
 std::optional<TypeBuilder::ErrorReason>
-validateType(HeapTypeInfo& info, std::unordered_set<HeapType>& seenTypes) {
+validateType(HeapTypeInfo& info,
+             std::unordered_set<HeapType>& seenTypes,
+             FeatureSet features) {
   if (auto* super = info.supertype) {
     // The supertype must be canonical (i.e. defined in a previous rec group)
     // or have already been defined in this rec group.
@@ -2416,6 +2423,9 @@ validateType(HeapTypeInfo& info, std::unordered_set<HeapType>& seenTypes) {
     }
   }
   if (auto* desc = info.described) {
+    if (!features.hasCustomDescriptors()) {
+      return TypeBuilder::ErrorReason::RequiresCustomDescriptors;
+    }
     if (info.kind != HeapTypeKind::Struct) {
       return TypeBuilder::ErrorReason::NonStructDescribes;
     }
@@ -2428,6 +2438,9 @@ validateType(HeapTypeInfo& info, std::unordered_set<HeapType>& seenTypes) {
     }
   }
   if (auto* desc = info.descriptor) {
+    if (!features.hasCustomDescriptors()) {
+      return TypeBuilder::ErrorReason::RequiresCustomDescriptors;
+    }
     if (info.kind != HeapTypeKind::Struct) {
       return TypeBuilder::ErrorReason::NonStructDescriptor;
     }
@@ -2544,7 +2557,8 @@ void updateReferencedHeapTypes(
 TypeBuilder::BuildResult
 buildRecGroup(std::unique_ptr<RecGroupInfo>&& groupInfo,
               std::vector<std::unique_ptr<HeapTypeInfo>>&& typeInfos,
-              std::unordered_map<HeapType, HeapType>& canonicalized) {
+              std::unordered_map<HeapType, HeapType>& canonicalized,
+              FeatureSet features) {
   // First, we need to replace any referenced temporary HeapTypes from
   // previously built groups with their canonicalized versions.
   for (auto& info : typeInfos) {
@@ -2555,7 +2569,7 @@ buildRecGroup(std::unique_ptr<RecGroupInfo>&& groupInfo,
   std::unordered_set<HeapType> seenTypes;
   for (size_t i = 0; i < typeInfos.size(); ++i) {
     auto& info = typeInfos[i];
-    if (auto err = validateType(*info, seenTypes)) {
+    if (auto err = validateType(*info, seenTypes, features)) {
       return {TypeBuilder::Error{i, *err}};
     }
     seenTypes.insert(asHeapType(info));
@@ -2661,8 +2675,10 @@ TypeBuilder::BuildResult TypeBuilder::build() {
       typeInfos.emplace_back(std::move(impl->entries[groupStart + i].info));
     }
 
-    auto built =
-      buildRecGroup(std::move(groupInfo), std::move(typeInfos), canonicalized);
+    auto built = buildRecGroup(std::move(groupInfo),
+                               std::move(typeInfos),
+                               canonicalized,
+                               impl->features);
     if (auto* error = built.getError()) {
       return {TypeBuilder::Error{groupStart + error->index, error->reason}};
     }

test/lit/validation/unused-descriptors.wast

@@ -0,0 +1,21 @@
+;; Test that we reject descriptor types when custom descriptors are not enabled,
+;; even if they are not directly used.
+
+;; RUN: not wasm-opt -all --disable-custom-descriptors %s 2>&1 | filecheck %s
+
+;; Check the binary parser, too.
+
+;; RUN: wasm-opt -all %s -o %t.wasm
+;; RUN: not wasm-opt -all --disable-custom-descriptors %t.wasm 2>&1 | filecheck %s
+
+;; CHECK: invalid type: custom descriptors required but not enabled
+
+(module
+  (rec
+    (type $struct (descriptor $desc (struct)))
+    (type $desc (describes $struct (struct)))
+    (type $used (struct))
+  )
+
+  (global $use (ref null $used) (ref.null none))
+)