Require unique_ptr to Module::addFunctionType() (#1672)

This fixes the memory leak in WasmBinaryBuilder::readSignatures() caused probably the exception thrown there before the FunctionType object is safe.

This also makes it clear that the Module becomes the owner of the FunctionType objects.

9 files changed, 23 insertions, 26 deletions

diff --git a/src/asmjs/asm_v_wasm.cpp b/src/asmjs/asm_v_wasm.cpp
index a937239f3..a0b3938fa 100644
--- a/src/asmjs/asm_v_wasm.cpp
+++ b/src/asmjs/asm_v_wasm.cpp
@@ -107,14 +107,13 @@ FunctionType* ensureFunctionType(std::string sig, Module* wasm) {
     return wasm->getFunctionType(name);
   }
   // add new type
-  auto type = new FunctionType;
+  auto type = make_unique<FunctionType>();
   type->name = name;
   type->result = sigToType(sig[0]);
   for (size_t i = 1; i < sig.size(); i++) {
     type->params.push_back(sigToType(sig[i]));
   }
-  wasm->addFunctionType(type);
-  return type;
+  return wasm->addFunctionType(std::move(type));
 }
 
 Expression* ensureDouble(Expression* expr, MixedArena& allocator) {
diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index 80f8d8276..9ed6ad314 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -306,7 +306,7 @@ void BinaryenModuleDispose(BinaryenModuleRef module) {
 
 BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const char* name, BinaryenType result, BinaryenType* paramTypes, BinaryenIndex numParams) {
   auto* wasm = (Module*)module;
-  auto* ret = new FunctionType;
+  auto ret = make_unique<FunctionType>();
   if (name) ret->name = name;
   else ret->name = Name::fromInt(wasm->functionTypes.size());
   ret->result = Type(result);
@@ -314,13 +314,6 @@ BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const
     ret->params.push_back(Type(paramTypes[i]));
   }
 
-  // Lock. This can be called from multiple threads at once, and is a
-  // point where they all access and modify the module.
-  {
-    std::lock_guard<std::mutex> lock(BinaryenFunctionTypeMutex);
-    wasm->addFunctionType(ret);
-  }
-
   if (tracing) {
     std::cout << "  {\n";
     std::cout << "    BinaryenType paramTypes[] = { ";
@@ -332,13 +325,16 @@ BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const
     std::cout << " };\n";
     size_t id = functionTypes.size();
     std::cout << "    functionTypes[" << id << "] = BinaryenAddFunctionType(the_module, ";
-    functionTypes[ret] = id;
+    functionTypes[ret.get()] = id;
     traceNameOrNULL(name);
     std::cout << ", " << result << ", paramTypes, " << numParams << ");\n";
     std::cout << "  }\n";
   }
 
-  return ret;
+  // Lock. This can be called from multiple threads at once, and is a
+  // point where they all access and modify the module.
+  std::lock_guard<std::mutex> lock(BinaryenFunctionTypeMutex);
+  return wasm->addFunctionType(std::move(ret));
 }
 void BinaryenRemoveFunctionType(BinaryenModuleRef module, const char* name) {
   if (tracing) {
diff --git a/src/ir/module-utils.h b/src/ir/module-utils.h
index c03015f41..b488db765 100644
--- a/src/ir/module-utils.h
+++ b/src/ir/module-utils.h
@@ -105,7 +105,7 @@ inline void copyModule(Module& in, Module& out) {
   // we use names throughout, not raw points, so simple copying is fine
   // for everything *but* expressions
   for (auto& curr : in.functionTypes) {
-    out.addFunctionType(new FunctionType(*curr));
+    out.addFunctionType(make_unique<FunctionType>(*curr));
   }
   for (auto& curr : in.exports) {
     out.addExport(new Export(*curr));
diff --git a/src/passes/LegalizeJSInterface.cpp b/src/passes/LegalizeJSInterface.cpp
index 9d223390e..8a68ccc88 100644
--- a/src/passes/LegalizeJSInterface.cpp
+++ b/src/passes/LegalizeJSInterface.cpp
@@ -189,7 +189,7 @@ private:
   // wasm calls the import, so it must call a stub that calls the actual legal JS import
   Name makeLegalStubForCalledImport(Function* im, Module* module) {
     Builder builder(*module);
-    auto* type = new FunctionType;
+    auto type = make_unique<FunctionType>();
     type->name =  Name(std::string("legaltype$") + im->name.str);
     auto* legal = new Function;
     legal->name = Name(std::string("legalimport$") + im->name.str);
@@ -236,13 +236,13 @@ private:
       type->result = imFunctionType->result;
     }
     func->result = imFunctionType->result;
-    FunctionTypeUtils::fillFunction(legal, type);
+    FunctionTypeUtils::fillFunction(legal, type.get());
 
     if (!module->getFunctionOrNull(func->name)) {
       module->addFunction(func);
     }
     if (!module->getFunctionTypeOrNull(type->name)) {
-      module->addFunctionType(type);
+      module->addFunctionType(std::move(type));
     }
     if (!module->getFunctionOrNull(legal->name)) {
       module->addFunction(legal);
diff --git a/src/tools/wasm-merge.cpp b/src/tools/wasm-merge.cpp
index e9ac6d649..6dd522d16 100644
--- a/src/tools/wasm-merge.cpp
+++ b/src/tools/wasm-merge.cpp
@@ -447,7 +447,7 @@ struct InputMergeable : public ExpressionStackWalker<InputMergeable, Visitor<Inp
 
     // copy in the data
     for (auto& curr : wasm.functionTypes) {
-      outputMergeable.wasm.addFunctionType(curr.release());
+      outputMergeable.wasm.addFunctionType(std::move(curr));
     }
     for (auto& curr : wasm.globals) {
       if (curr->imported()) {
diff --git a/src/wasm.h b/src/wasm.h
index 27a302d3c..ec8722ad0 100644
--- a/src/wasm.h
+++ b/src/wasm.h
@@ -917,7 +917,7 @@ public:
   Function* getFunctionOrNull(Name name);
   Global* getGlobalOrNull(Name name);
 
-  void addFunctionType(FunctionType* curr);
+  FunctionType* addFunctionType(std::unique_ptr<FunctionType> curr);
   void addExport(Export* curr);
   void addFunction(Function* curr);
   void addGlobal(Global* curr);
diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
index af42ed8a4..a78636c92 100644
--- a/src/wasm/wasm-binary.cpp
+++ b/src/wasm/wasm-binary.cpp
@@ -937,7 +937,7 @@ void WasmBinaryBuilder::readSignatures() {
   if (debug) std::cerr << "num: " << numTypes << std::endl;
   for (size_t i = 0; i < numTypes; i++) {
     if (debug) std::cerr << "read one" << std::endl;
-    auto curr = new FunctionType;
+    auto curr = make_unique<FunctionType>();
     auto form = getS32LEB();
     if (form != BinaryConsts::EncodedType::Func) {
       throwError("bad signature form " + std::to_string(form));
@@ -957,7 +957,7 @@ void WasmBinaryBuilder::readSignatures() {
       curr->result = getType();
     }
     curr->name = Name::fromInt(wasm.functionTypes.size());
-    wasm.addFunctionType(curr);
+    wasm.addFunctionType(std::move(curr));
   }
 }
 
diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index 0dfea962b..f5280bc33 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -421,7 +421,7 @@ void SExpressionWasmBuilder::preParseFunctionType(Element& s) {
       functionType->name = Name::fromInt(wasm.functionTypes.size());
       functionTypeNames.push_back(functionType->name);
       if (wasm.getFunctionTypeOrNull(functionType->name)) throw ParseException("duplicate function type", s.line, s.col);
-      wasm.addFunctionType(functionType.release());
+      wasm.addFunctionType(std::move(functionType));
     }
   }
 }
@@ -1828,7 +1828,7 @@ void SExpressionWasmBuilder::parseType(Element& s) {
   }
   functionTypeNames.push_back(type->name);
   if (wasm.getFunctionTypeOrNull(type->name)) throw ParseException("duplicate function type", s.line, s.col);
-  wasm.addFunctionType(type.release());
+  wasm.addFunctionType(std::move(type));
 }
 
 } // namespace wasm
diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp
index cfee4f3c4..757bc1828 100644
--- a/src/wasm/wasm.cpp
+++ b/src/wasm/wasm.cpp
@@ -809,15 +809,17 @@ Global* Module::getGlobalOrNull(Name name) {
   return iter->second;
 }
 
-void Module::addFunctionType(FunctionType* curr) {
+FunctionType* Module::addFunctionType(std::unique_ptr<FunctionType> curr) {
   if (!curr->name.is()) {
     Fatal() << "Module::addFunctionType: empty name";
   }
   if (getFunctionTypeOrNull(curr->name)) {
     Fatal() << "Module::addFunctionType: " << curr->name << " already exists";
   }
-  functionTypes.push_back(std::unique_ptr<FunctionType>(curr));
-  functionTypesMap[curr->name] = curr;
+  auto* p = curr.get();
+  functionTypes.emplace_back(std::move(curr));
+  functionTypesMap[p->name] = p;
+  return p;
 }
 
 void Module::addExport(Export* curr) {