[WebAssembly] Validate sections: order, and presence of required CODE

[ncw]

Summary

No test changes.

---

Split out of D43940 as requested - fixing tests is independent of the validation

[ncw]

This is stalled, until we agree on whether to actually check the order of the sections, or break it down into logic to check less generally that certain sections come after others; and secondly whether the order of all the sections even should be specified.

I've raised a GitHub issue to gather further feedback: https://github.com/WebAssembly/tool-conventions/issues/45

[aheejin]

D54924, which tries to check similar things, has been landed. Let me know if you have any more suggestions.

[aheejin]

Also more related patches: D58312

include/llvm/Object/Wasm.h

private:
 
   const WasmSection &getWasmSection(DataRefImpl Ref) const;
   const wasm::WasmRelocation &getWasmRelocation(DataRefImpl Ref) const;
 
   WasmSection* findCustomSectionByName(StringRef Name);
   WasmSection* findSectionByType(uint32_t Type);
 
   const uint8_t *getPtr(size_t Offset) const;
+  Error checkSectionOrder(const WasmSection &Section, int &LastSecOrder);
+  Error checkCodeSection();
   Error parseSection(WasmSection &Sec);
   Error parseCustomSection(WasmSection &Sec, const uint8_t *Ptr,
                            const uint8_t *End);
 
   // Standard section types
   Error parseTypeSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseImportSection(const uint8_t *Ptr, const uint8_t *End);
   Error parseFunctionSection(const uint8_t *Ptr, const uint8_t *End);
private:
   std::vector<wasm::WasmImport> Imports;
   std::vector<wasm::WasmExport> Exports;
   std::vector<wasm::WasmElemSegment> ElemSegments;
   std::vector<WasmSegment> DataSegments;
   std::vector<wasm::WasmFunction> Functions;
   std::vector<WasmSymbol> Symbols;
   std::vector<StringRef> Comdats;
   std::vector<wasm::WasmFunctionName> DebugNames;
-  uint32_t StartFunction = -1;
+  uint32_t StartFunction = UINT32_MAX;
   bool HasLinkingSection = false;
   wasm::WasmLinkingData LinkingData;
   uint32_t NumImportedGlobals = 0;
   uint32_t NumImportedFunctions = 0;
-  uint32_t CodeSection = 0;
-  uint32_t DataSection = 0;
-  uint32_t GlobalSection = 0;
+  uint32_t CodeSection = UINT32_MAX;
+  uint32_t DataSection = UINT32_MAX;
+  uint32_t GlobalSection = UINT32_MAX;
 };
 
 } // end namespace object
 
 inline raw_ostream &operator<<(raw_ostream &OS,
                                const object::WasmSymbol &Sym) {
   Sym.print(OS);
   return OS;
 }
 
 } // end namespace llvm
 
 #endif // LLVM_OBJECT_WASM_H

lib/Object/WasmObjectFile.cpp

static Error readSection(WasmSection &Section, const uint8_t *&Ptr,
   Section.Type = readUint8(Ptr);
   uint32_t Size = readVaruint32(Ptr);
   if (Size == 0)
     return make_error<StringError>("Zero length section",
                                    object_error::parse_failed);
   if (Ptr + Size > Eof)
     return make_error<StringError>("Section too large",
                                    object_error::parse_failed);
+  if (Section.Type == wasm::WASM_SEC_CUSTOM) {
+    const uint8_t* NamePtr = Ptr;
+    Section.Name = readString(NamePtr);
+    if (NamePtr > Ptr + Size)
+      return make_error<StringError>("Section name too large",
+                                     object_error::parse_failed);
+  }
   Section.Content = ArrayRef<uint8_t>(Ptr, Size);
   Ptr += Size;
   return Error::success();
 }
 
+// For core Wasm sections, there is a defined order for all sections, and custom
+// sections also (should) define where they fit into that order.  We return -1
+// for custom sections whose order we don't know.
+enum : unsigned {
+  // The order of standard sections is precisely given by the spec.
+  WASM_SEC_ORDER_TYPE      = 10,
+  WASM_SEC_ORDER_IMPORT    = 20,
+//  WASM_SEC_ORDER_EXCEPTION = 30,
+  WASM_SEC_ORDER_FUNCTION  = 40,
+  WASM_SEC_ORDER_TABLE     = 50,
+  WASM_SEC_ORDER_MEMORY    = 60,
+  WASM_SEC_ORDER_GLOBAL    = 70,
+  WASM_SEC_ORDER_EXPORT    = 80,
+  WASM_SEC_ORDER_START     = 90,
+  WASM_SEC_ORDER_ELEM      = 100,
+  WASM_SEC_ORDER_CODE      = 110,
+  WASM_SEC_ORDER_DATA      = 120,
+  // "linking" requires DATA section in order to validate data symbols
+  WASM_SEC_ORDER_LINKING   = 125,
+  // Must come after "linking" in order to validate reloc indexes.
+  WASM_SEC_ORDER_RELOC     = 126,
+  // Spec: "name" MUST appear after DATA.  Comes after "linking" to allow
+  // symbol table to set default function name.
+  WASM_SEC_ORDER_NAME      = 130,
+};
+static int getSectionOrder(const WasmSection &Sec) {
+  switch (Sec.Type) {
+  case wasm::WASM_SEC_CUSTOM:
+    if (Sec.Name == "linking")
+      return WASM_SEC_ORDER_LINKING;
+    if (Sec.Name.startswith("reloc."))
+      return WASM_SEC_ORDER_RELOC;
+    if (Sec.Name == "name")
+      return WASM_SEC_ORDER_NAME;
+    return -1;
+  case wasm::WASM_SEC_TYPE:      return WASM_SEC_ORDER_TYPE;
+  case wasm::WASM_SEC_IMPORT:    return WASM_SEC_ORDER_IMPORT;
+//  case wasm::WASM_SEC_EXCEPTION: return WASM_SEC_ORDER_EXCEPTION;
+  case wasm::WASM_SEC_FUNCTION:  return WASM_SEC_ORDER_FUNCTION;
+  case wasm::WASM_SEC_TABLE:     return WASM_SEC_ORDER_TABLE;
+  case wasm::WASM_SEC_MEMORY:    return WASM_SEC_ORDER_MEMORY;
+  case wasm::WASM_SEC_GLOBAL:    return WASM_SEC_ORDER_GLOBAL;
+  case wasm::WASM_SEC_EXPORT:    return WASM_SEC_ORDER_EXPORT;
+  case wasm::WASM_SEC_START:     return WASM_SEC_ORDER_START;
+  case wasm::WASM_SEC_ELEM:      return WASM_SEC_ORDER_ELEM;
+  case wasm::WASM_SEC_CODE:      return WASM_SEC_ORDER_CODE;
+  case wasm::WASM_SEC_DATA:      return WASM_SEC_ORDER_DATA;
+  default:
+    llvm_unreachable("invalid section");
+  }
+}
+
 WasmObjectFile::WasmObjectFile(MemoryBufferRef Buffer, Error &Err)
     : ObjectFile(Binary::ID_Wasm, Buffer) {
   ErrorAsOutParameter ErrAsOutParam(&Err);
   Header.Magic = getData().substr(0, 4);
   if (Header.Magic != StringRef("\0asm", 4)) {
     Err = make_error<StringError>("Bad magic number",
                                   object_error::parse_failed);
     return;
WasmObjectFile::WasmObjectFile(MemoryBufferRef Buffer, Error &Err)
   Header.Version = readUint32(Ptr);
   if (Header.Version != wasm::WasmVersion) {
     Err = make_error<StringError>("Bad version number",
                                   object_error::parse_failed);
     return;
   }
 
   WasmSection Sec;
+  int LastSecOrder = 0;
   while (Ptr < Eof) {
     if ((Err = readSection(Sec, Ptr, getPtr(0), Eof)))
       return;
+    if ((Err = checkSectionOrder(Sec, LastSecOrder)))
+      return;
     if ((Err = parseSection(Sec)))
       return;
 
     Sections.push_back(Sec);
   }
+
+  Err = checkCodeSection();
+}
+
+Error WasmObjectFile::checkSectionOrder(const WasmSection &Section,
+                                        int &LastSecOrder) {
+  int SecOrder = getSectionOrder(Section);
+  if (SecOrder != -1) {
+    // Disallow duplicate sections (except for "reloc.") as well as
+    // out-of-order sections.
+    if (SecOrder < LastSecOrder ||
+        (SecOrder != (int)WASM_SEC_ORDER_RELOC && SecOrder == LastSecOrder))
+      return make_error<GenericBinaryError>("Illegal section order",
+                                            object_error::parse_failed);
+    // If an empty FUNCTION section is present, there's no need for an empty
+    // CODE section to match, the vectors just have to be the same length.
+    // We must check the CODE section was present if required after skipping
+    // past it.
+    if (LastSecOrder < (int)WASM_SEC_ORDER_CODE &&
+        SecOrder > (int)WASM_SEC_ORDER_CODE) {
+      if (Error Err = checkCodeSection())
+        return Err;
+    }
+
+    LastSecOrder = SecOrder;
+  }
+  return Error::success();
+}
+
+Error WasmObjectFile::checkCodeSection() {
+  if (FunctionTypes.size() != Functions.size())
+    return make_error<GenericBinaryError>("Expected CODE section",
+                                          object_error::parse_failed);
+  return Error::success();
 }
 
 Error WasmObjectFile::parseSection(WasmSection &Sec) {
   const uint8_t* Start = Sec.Content.data();
   const uint8_t* End = Start + Sec.Content.size();
   switch (Sec.Type) {
   case wasm::WASM_SEC_CUSTOM:
     return parseCustomSection(Sec, Start, End);
Error WasmObjectFile::parseSection(WasmSection &Sec) {
   default:
     return make_error<GenericBinaryError>("Bad section type",
                                           object_error::parse_failed);
   }
 }
 
 Error WasmObjectFile::parseNameSection(const uint8_t *Ptr, const uint8_t *End) {
   llvm::DenseSet<uint64_t> Seen;
-  if (Functions.size() != FunctionTypes.size()) {
-    return make_error<GenericBinaryError>("Names must come after code section",
-                                          object_error::parse_failed);
-  }
-
   while (Ptr < End) {
     uint8_t Type = readUint8(Ptr);
     uint32_t Size = readVaruint32(Ptr);
     const uint8_t *SubSectionEnd = Ptr + Size;
     switch (Type) {
     case wasm::WASM_NAMES_FUNCTION: {
       uint32_t Count = readVaruint32(Ptr);
       while (Count--) {
if (Ptr != End)
     return make_error<GenericBinaryError>("Name section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
 Error WasmObjectFile::parseLinkingSection(const uint8_t *Ptr,
                                           const uint8_t *End) {
   HasLinkingSection = true;
-  if (Functions.size() != FunctionTypes.size()) {
-    return make_error<GenericBinaryError>(
-        "Linking data must come after code section", object_error::parse_failed);
-  }
-
   while (Ptr < End) {
     uint8_t Type = readUint8(Ptr);
     uint32_t Size = readVaruint32(Ptr);
     const uint8_t *SubSectionEnd = Ptr + Size;
     switch (Type) {
     case wasm::WASM_SYMBOL_TABLE:
       if (Error Err = parseLinkingSectionSymtab(Ptr, SubSectionEnd))
         return Err;
Error WasmObjectFile::parseRelocSection(StringRef Name, const uint8_t *Ptr,
   if (Ptr != End)
     return make_error<GenericBinaryError>("Reloc section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
 Error WasmObjectFile::parseCustomSection(WasmSection &Sec,
                                          const uint8_t *Ptr, const uint8_t *End) {
-  Sec.Name = readString(Ptr);
+  Ptr = Sec.Name.bytes_end();
   if (Sec.Name == "name") {
     if (Error Err = parseNameSection(Ptr, End))
       return Err;
   } else if (Sec.Name == "linking") {
     if (Error Err = parseLinkingSection(Ptr, End))
       return Err;
   } else if (Sec.Name.startswith("reloc.")) {
     if (Error Err = parseRelocSection(Sec.Name, Ptr, End))