Require unique_ptr to Module::addFunctionType()

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.

Author: chfast

src/asmjs/asm_v_wasm.cpp

@@ -99,14 +99,13 @@ FunctionType* ensureFunctionType(std::string sig, Module* wasm) {
     return wasm->getFunctionType(name);
   }
   // add new type
-  auto type = new FunctionType;
+  std::unique_ptr<FunctionType> type{new 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) {

src/binaryen-c.cpp

@@ -190,41 +190,42 @@ void BinaryenModuleDispose(BinaryenModuleRef module) {
 
 // Function types
 
-BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const char* name, BinaryenType result, BinaryenType* paramTypes, BinaryenIndex numParams) {
+BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const char* name,
+  BinaryenType result, BinaryenType* paramTypes, BinaryenIndex numParams) {
   auto* wasm = (Module*)module;
-  auto* ret = new FunctionType;
-  if (name) ret->name = name;
-  else ret->name = Name::fromInt(wasm->functionTypes.size());
+  std::unique_ptr<FunctionType> ret{new FunctionType};
+  if (name)
+    ret->name = name;
+  else
+    ret->name = Name::fromInt(wasm->functionTypes.size());
   ret->result = Type(result);
   for (BinaryenIndex i = 0; i < numParams; i++) {
     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[] = { ";
     for (BinaryenIndex i = 0; i < numParams; i++) {
-      if (i > 0) std::cout << ", ";
+      if (i > 0)
+        std::cout << ", ";
       std::cout << paramTypes[i];
     }
-    if (numParams == 0) std::cout << "0"; // ensure the array is not empty, otherwise a compiler error on VS
+    if (numParams == 0)
+      std::cout << "0"; // ensure the array is not empty, otherwise a compiler error on VS
     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) {

src/ir/module-utils.h

@@ -73,7 +73,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(std::unique_ptr<FunctionType>{new FunctionType(*curr)});
   }
   for (auto& curr : in.imports) {
     out.addImport(new Import(*curr));

src/passes/LegalizeJSInterface.cpp

@@ -181,7 +181,7 @@ struct LegalizeJSInterface : public Pass {
   // wasm calls the import, so it must call a stub that calls the actual legal JS import
   Import* makeLegalStub(Import* im, Module* module, Name& funcName) {
     Builder builder(*module);
-    auto* type = new FunctionType();
+    std::unique_ptr<FunctionType> type{new FunctionType()};
     type->name =  Name(std::string("legaltype$") + im->name.str);
     auto* legal = new Import();
     legal->name = Name(std::string("legalimport$") + im->name.str);
@@ -233,7 +233,7 @@ struct LegalizeJSInterface : public Pass {
     func->result = imFunctionType->result;
 
     module->addFunction(func);
-    module->addFunctionType(type);
+    module->addFunctionType(std::move(type));
     return legal;
   }
 

src/tools/wasm-merge.cpp

@@ -446,7 +446,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.imports) {
       if (curr->kind == ExternalKind::Memory || curr->kind == ExternalKind::Table) {

src/wasm.h

@@ -786,7 +786,7 @@ class Module {
   Function* getFunctionOrNull(Name name);
   Global* getGlobalOrNull(Name name);
 
-  void addFunctionType(FunctionType* curr);
+  FunctionType* addFunctionType(std::unique_ptr<FunctionType> curr);
   void addImport(Import* curr);
   void addExport(Export* curr);
   void addFunction(Function* curr);

src/wasm/wasm-binary.cpp

@@ -909,7 +909,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;
+    std::unique_ptr<FunctionType> curr{new FunctionType};
     auto form = getS32LEB();
     if (form != BinaryConsts::EncodedType::Func) {
       throwError("bad signature form " + std::to_string(form));
@@ -929,7 +929,7 @@ void WasmBinaryBuilder::readSignatures() {
       curr->result = getType();
     }
     curr->name = Name::fromInt(wasm.functionTypes.size());
-    wasm.addFunctionType(curr);
+    wasm.addFunctionType(std::move(curr));
   }
 }
 

src/wasm/wasm-s-parser.cpp

@@ -419,7 +419,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));
     }
   }
 }
@@ -1965,7 +1965,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

src/wasm/wasm.cpp

@@ -722,15 +722,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[curr->name] = p;
+  return p;
 }
 
 void Module::addImport(Import* curr) {