Remove hashbrown and use BTree{Map,Set} from the alloc crate (#187)

* Remove hashbrown and use BTree{Map,Set} from the alloc crate

wasmi-validation must handle untrusted input and hence we switch from
Hash{Set,Map} (whether std's or hashbrown's) to BTree{Set,Map} to avoid
algorithmic complexity attacks while retaining no_std support.

Closes #183

* Improve memory locality of checking for duplicate exports

Using a sorted slice gives us the same O(N log N) worst case execution
time as using a BTreeMap, but using a single allocation as with HashMap,
so that we should see better memory locality and hence better constant
factors when checking for duplicate exports.

Cargo.toml

@@ -13,7 +13,6 @@ exclude = [ "/res/*", "/tests/*", "/fuzz/*", "/benches/*" ]
 [dependencies]
 wasmi-validation = { version = "0.1", path = "validation", default-features = false }
 parity-wasm = { version = "0.31", default-features = false }
-hashbrown = { version = "0.1.8", optional = true }
 memory_units = "0.3.0"
 libm = { version = "0.1.2", optional = true }
 
@@ -30,10 +29,8 @@ std = [
     "wasmi-validation/std",
 ]
 # Enable for no_std support
-# hashbrown only works on no_std
 core = [
     "wasmi-validation/core",
-    "hashbrown/nightly",
     "libm"
 ]
 

src/imports.rs

@@ -1,10 +1,7 @@
 #[allow(unused_imports)]
 use alloc::prelude::v1::*;
 
-#[cfg(not(feature = "std"))]
-use hashbrown::HashMap;
-#[cfg(feature = "std")]
-use std::collections::HashMap;
+use alloc::collections::BTreeMap;
 
 use func::FuncRef;
 use global::GlobalRef;
@@ -106,7 +103,7 @@ pub trait ImportResolver {
 /// [`ImportResolver`]: trait.ImportResolver.html
 /// [`ModuleImportResolver`]: trait.ModuleImportResolver.html
 pub struct ImportsBuilder<'a> {
-    modules: HashMap<String, &'a ModuleImportResolver>,
+    modules: BTreeMap<String, &'a ModuleImportResolver>,
 }
 
 impl<'a> Default for ImportsBuilder<'a> {
@@ -119,7 +116,7 @@ impl<'a> ImportsBuilder<'a> {
     /// Create an empty `ImportsBuilder`.
     pub fn new() -> ImportsBuilder<'a> {
         ImportsBuilder {
-            modules: HashMap::new(),
+            modules: BTreeMap::new(),
         }
     }
 

src/lib.rs

@@ -114,8 +114,6 @@ extern crate assert_matches;
 #[cfg(test)]
 extern crate wabt;
 
-#[cfg(not(feature = "std"))]
-extern crate hashbrown;
 extern crate memory_units as memory_units_crate;
 extern crate parity_wasm;
 

src/module.rs

@@ -5,10 +5,7 @@ use core::cell::RefCell;
 use core::fmt;
 use Trap;
 
-#[cfg(not(feature = "std"))]
-use hashbrown::HashMap;
-#[cfg(feature = "std")]
-use std::collections::HashMap;
+use alloc::collections::BTreeMap;
 
 use core::cell::Ref;
 use func::{FuncBody, FuncInstance, FuncRef};
@@ -162,7 +159,7 @@ pub struct ModuleInstance {
     funcs: RefCell<Vec<FuncRef>>,
     memories: RefCell<Vec<MemoryRef>>,
     globals: RefCell<Vec<GlobalRef>>,
-    exports: RefCell<HashMap<String, ExternVal>>,
+    exports: RefCell<BTreeMap<String, ExternVal>>,
 }
 
 impl ModuleInstance {
@@ -173,7 +170,7 @@ impl ModuleInstance {
             tables: RefCell::new(Vec::new()),
             memories: RefCell::new(Vec::new()),
             globals: RefCell::new(Vec::new()),
-            exports: RefCell::new(HashMap::new()),
+            exports: RefCell::new(BTreeMap::new()),
         }
     }
 

validation/Cargo.toml

@@ -9,7 +9,6 @@ description = "Wasm code validator"
 
 [dependencies]
 parity-wasm = { version = "0.31", default-features = false }
-hashbrown = { version = "0.1.8", optional = true }
 
 [dev-dependencies]
 assert_matches = "1.1"
@@ -17,6 +16,4 @@ assert_matches = "1.1"
 [features]
 default = ["std"]
 std = ["parity-wasm/std"]
-core = [
-    "hashbrown/nightly"
-]
+core = []

validation/src/lib.rs

@@ -27,15 +27,10 @@ use core::fmt;
 #[cfg(feature = "std")]
 use std::error;
 
-#[cfg(not(feature = "std"))]
-use hashbrown::HashSet;
-#[cfg(feature = "std")]
-use std::collections::HashSet;
-
 use self::context::ModuleContextBuilder;
 use parity_wasm::elements::{
-    BlockType, External, FuncBody, GlobalEntry, GlobalType, InitExpr, Instruction, Internal,
-    MemoryType, Module, ResizableLimits, TableType, Type, ValueType,
+    BlockType, ExportEntry, External, FuncBody, GlobalEntry, GlobalType, InitExpr, Instruction,
+    Internal, MemoryType, Module, ResizableLimits, TableType, Type, ValueType,
 };
 
 pub mod context;
@@ -250,13 +245,21 @@ pub fn validate_module<V: Validator>(module: &Module) -> Result<V::Output, Error
 
     // validate export section
     if let Some(export_section) = module.export_section() {
-        let mut export_names = HashSet::with_capacity(export_section.entries().len());
-        for export in export_section.entries() {
-            // HashSet::insert returns false if item already in set.
-            let duplicate = export_names.insert(export.field()) == false;
-            if duplicate {
-                return Err(Error(format!("duplicate export {}", export.field())));
+        let mut export_names = export_section
+            .entries()
+            .iter()
+            .map(ExportEntry::field)
+            .collect::<Vec<_>>();
+
+        export_names.sort_unstable();
+
+        for (fst, snd) in export_names.iter().zip(export_names.iter().skip(1)) {
+            if fst == snd {
+                return Err(Error(format!("duplicate export {}", fst)));
             }
+        }
+
+        for export in export_section.entries() {
             match *export.internal() {
                 Internal::Function(function_index) => {
                     context.require_function(function_index)?;