Fix memory leak in WASM

* add wasm_extern_vec_delete_with_size api to handle exception cases during the imports reading
* wasm_extern_vec_t is now correctly deallocated

Signed-off-by: HyukWoo Park <hyukwoo.park@samsung.com>

.github/workflows/es-actions.yml

@@ -136,8 +136,8 @@ jobs:
     - name: Patch WABT
       working-directory: ./third_party/wasm/wabt
       run: |
-        cp ../../../tools/test/wasm-js/func_index_patch .
-        patch -p0 < func_index_patch
+        cp ../../../tools/test/wasm-js/wabt_patch .
+        patch -p0 < wabt_patch
     - name: Build x86
       env:
         BUILD_OPTIONS: -DESCARGOT_HOST=linux -DESCARGOT_ARCH=x86 -DESCARGOT_MODE=debug -DESCARGOT_LIBICU_SUPPORT_WITH_DLOPEN=OFF -DESCARGOT_WASM=ON -DESCARGOT_OUTPUT=shell_test -GNinja
@@ -188,8 +188,8 @@ jobs:
     - name: Patch WABT
       working-directory: ./third_party/wasm/wabt
       run: |
-        cp ../../../tools/test/wasm-js/func_index_patch .
-        patch -p0 < func_index_patch
+        cp ../../../tools/test/wasm-js/wabt_patch .
+        patch -p0 < wabt_patch
     - name: Download Coverity Tool
       env:
         TOKEN: ${{ secrets.COVERITY_SCAN_TOKEN }}

src/wasm/GlobalObjectBuiltinWASM.cpp

@@ -50,7 +50,7 @@ static Value builtinWASMValidate(ExecutionState& state, Value thisValue, size_t
     ASSERT(!srcBuffer->isDetachedBuffer());
     size_t byteLength = srcBuffer->byteLength();
 
-    wasm_byte_vec_t binary;
+    own wasm_byte_vec_t binary;
     wasm_byte_vec_new_uninitialized(&binary, byteLength);
     memcpy(binary.data, srcBuffer->data(), byteLength);
 
@@ -124,9 +124,10 @@ static Value builtinWASMInstantiate(ExecutionState& state, Value thisValue, size
         Value reason[] = { readImportsResult.error };
         Object::call(state, capability.m_rejectFunction, Value(), 1, reason);
     } else {
-        // FIXME pass imports data into ExtendedNativeFunctionObjectImpl and delete imports data inside ExtendedNativeFunctionObjectImpl
-        auto moduleInstantiator = new ExtendedNativeFunctionObjectImpl<1>(state, NativeFunctionInfo(AtomicString(), wasmInstantiateCoreModule, 1, NativeFunctionInfo::Strict));
-        moduleInstantiator->setInternalSlotAsPointer(0, imports.data);
+        // Note) pass imports data and its size into ExtendedNativeFunctionObjectImpl and delete imports vector inside ExtendedNativeFunctionObjectImpl call
+        auto moduleInstantiator = new ExtendedNativeFunctionObjectImpl<2>(state, NativeFunctionInfo(AtomicString(), wasmInstantiateCoreModule, 1, NativeFunctionInfo::Strict));
+        moduleInstantiator->setInternalSlot(0, Value(imports.size));
+        moduleInstantiator->setInternalSlotAsPointer(1, imports.data);
 
         Job* job = new PromiseReactionJob(state.context(), PromiseReaction(moduleInstantiator, capability), firstArg);
         state.context()->vmInstance()->enqueuePromiseJob(capability.m_promise->asPromiseObject(), job);
@@ -288,11 +289,7 @@ static Value builtinWASMInstanceConstructor(ExecutionState& state, Value thisVal
     // Instantiate the core of a WebAssembly module module with imports, and let instance be the result.
     own wasm_trap_t* trap = nullptr;
     own wasm_instance_t* instance = wasm_instance_new(state.context()->vmInstance()->wasmStore(), module, imports.data, &trap);
-
-    // FIXME should not call destructor of each element in imports
-    // wasm_extern_vec_delete(&imports);
-    delete[] imports.data;
-    imports.size = 0;
+    wasm_extern_vec_delete(&imports);
 
     // TODO error exception
     if (!instance) {
@@ -367,10 +364,10 @@ static Value builtinWASMMemoryConstructor(ExecutionState& state, Value thisValue
 
     // Let memtype be { min initial, max maximum }
     wasm_limits_t limits = { initial, maximum };
-    wasm_memorytype_t* memtype = wasm_memorytype_new(&limits);
+    own wasm_memorytype_t* memtype = wasm_memorytype_new(&limits);
 
     // Let (store, memaddr) be mem_alloc(store, memtype). If allocation fails, throw a RangeError exception.
-    wasm_memory_t* memaddr = wasm_memory_new(state.context()->vmInstance()->wasmStore(), memtype);
+    own wasm_memory_t* memaddr = wasm_memory_new(state.context()->vmInstance()->wasmStore(), memtype);
     wasm_memorytype_delete(memtype);
 
     wasm_ref_t* memref = wasm_memory_as_ref(memaddr);
@@ -539,7 +536,7 @@ static Value builtinWASMTableConstructor(ExecutionState& state, Value thisValue,
     own wasm_tabletype_t* tabletype = wasm_tabletype_new(wasm_valtype_new(WASM_FUNCREF), &limits);
 
     // Let (store, tableaddr) be table_alloc(store, type).
-    wasm_table_t* tableaddr = wasm_table_new(state.context()->vmInstance()->wasmStore(), tabletype, nullptr);
+    own wasm_table_t* tableaddr = wasm_table_new(state.context()->vmInstance()->wasmStore(), tabletype, nullptr);
     wasm_tabletype_delete(tabletype);
 
     wasm_ref_t* tableref = wasm_table_as_ref(tableaddr);
@@ -618,16 +615,16 @@ static Value builtinWASMTableGet(ExecutionState& state, Value thisValue, size_t
     // Let tableaddr be this.[[Table]].
     wasm_table_t* tableaddr = thisValue.asObject()->asWASMTableObject()->table();
 
-    // Let result be table_read(store, tableaddr, index).
-    wasm_ref_t* result = wasm_table_get(tableaddr, index);
-
     // If result is error, throw a RangeError exception.
-    // FIXME check the error by comparing the size
+    // FIXME check the error by comparing the size in advance
     if (index >= wasm_table_size(tableaddr)) {
         ErrorObject::throwBuiltinError(state, ErrorObject::RangeError, strings->WebAssemblyDotTable.string(), false, strings->get.string(), ErrorObject::Messages::GlobalObject_RangeError);
     }
 
-    // FIXME for the case of empty function (nullptr)
+    // Let result be table_read(store, tableaddr, index).
+    own wasm_ref_t* result = wasm_table_get(tableaddr, index);
+
+    // FIXME wasm_table_get returns nullptr for the empty function and error case both
     if (!result) {
         return Value(Value::Null);
     }
@@ -639,11 +636,13 @@ static Value builtinWASMTableGet(ExecutionState& state, Value thisValue, size_t
     for (auto iter = map.begin(); iter != map.end(); iter++) {
         wasm_ref_t* ref = iter->first;
         if (wasm_ref_same(result, ref)) {
+            wasm_ref_delete(result);
             return iter->second;
         }
     }
 
     ASSERT_NOT_REACHED();
+    wasm_ref_delete(result);
     return Value();
 }
 
@@ -743,7 +742,7 @@ static Value builtinWASMGlobalConstructor(ExecutionState& state, Value thisValue
     }
 
     Value valueArg = argc > 1 ? argv[1] : Value();
-    wasm_val_t value;
+    own wasm_val_t value;
     if (valueArg.isUndefined()) {
         // let value be DefaultValue(valuetype).
         value = WASMValueConverter::wasmDefaultValue(valuetype);
@@ -753,10 +752,11 @@ static Value builtinWASMGlobalConstructor(ExecutionState& state, Value thisValue
     }
 
     // If mutable is true, let globaltype be var valuetype; otherwise, let globaltype be const valuetype.
-    wasm_globaltype_t* globaltype = wasm_globaltype_new(wasm_valtype_new(valuetype), mut);
+    // Note) value should not have any reference in itself, so we don't have to call `wasm_val_delete`
+    own wasm_globaltype_t* globaltype = wasm_globaltype_new(wasm_valtype_new(valuetype), mut);
 
     // Let (store, globaladdr) be global_alloc(store, globaltype, value).
-    wasm_global_t* globaladdr = wasm_global_new(state.context()->vmInstance()->wasmStore(), globaltype, &value);
+    own wasm_global_t* globaladdr = wasm_global_new(state.context()->vmInstance()->wasmStore(), globaltype, &value);
     wasm_globaltype_delete(globaltype);
 
     wasm_ref_t* globalref = wasm_global_as_ref(globaladdr);
@@ -811,19 +811,19 @@ static Value builtinWASMGlobalValueSetter(ExecutionState& state, Value thisValue
     wasm_global_t* globaladdr = thisValue.asObject()->asWASMGlobalObject()->global();
 
     // Let mut valuetype be global_type(store, globaladdr).
-    wasm_globaltype_t* globaltype = wasm_global_type(globaladdr);
+    own wasm_globaltype_t* globaltype = wasm_global_type(globaladdr);
     wasm_mutability_t mut = wasm_globaltype_mutability(globaltype);
     wasm_valkind_t valuetype = wasm_valtype_kind(wasm_globaltype_content(globaltype));
+    wasm_globaltype_delete(globaltype);
 
     // If mut is const, throw a TypeError.
     if (mut == WASM_CONST) {
-        wasm_globaltype_delete(globaltype);
         ErrorObject::throwBuiltinError(state, ErrorObject::TypeError, state.context()->staticStrings().WebAssemblyDotGlobal.string(), false, state.context()->staticStrings().value.string(), ErrorObject::Messages::WASM_SetToGlobalConstValue);
     }
-    wasm_globaltype_delete(globaltype);
 
     // Let value be ToWebAssemblyValue(the given value, valuetype).
-    wasm_val_t value = WASMValueConverter::wasmToWebAssemblyValue(state, argv[0], valuetype);
+    // Note) value should not have any reference in itself, so we don't have to call `wasm_val_delete`
+    own wasm_val_t value = WASMValueConverter::wasmToWebAssemblyValue(state, argv[0], valuetype);
 
     // Let store be global_write(store, globaladdr, value).
     // TODO If store is error, throw a RangeError exception.

src/wasm/WASMBuiltinOperations.h

@@ -126,11 +126,11 @@ static Value wasmCompileModule(ExecutionState& state, Value thisValue, size_t ar
     ASSERT(!srcBuffer->isDetachedBuffer());
     size_t byteLength = srcBuffer->byteLength();
 
-    wasm_byte_vec_t binary;
+    own wasm_byte_vec_t binary;
     wasm_byte_vec_new_uninitialized(&binary, byteLength);
     memcpy(binary.data, srcBuffer->data(), byteLength);
 
-    wasm_module_t* module = wasm_module_new(state.context()->vmInstance()->wasmStore(), &binary);
+    own wasm_module_t* module = wasm_module_new(state.context()->vmInstance()->wasmStore(), &binary);
     wasm_byte_vec_delete(&binary);
 
     if (!module) {
@@ -222,18 +222,18 @@ static own wasm_trap_t* callbackHostFunction(void* env, const wasm_val_t args[],
     return nullptr;
 }
 
-static wasm_func_t* wasmCreateHostFunction(ExecutionState& state, Object* func, wasm_functype_t* functype)
+static own wasm_func_t* wasmCreateHostFunction(ExecutionState& state, Object* func, wasm_functype_t* functype)
 {
     // Assert: IsCallable(func).
     ASSERT(func->isCallable());
 
     // Let store be the surrounding agent's associated store.
     // Let (store, funcaddr) be func_alloc(store, functype, hostfunc).
-    // NOTE we should clone functype here because functype needs to be maintained for host function call later.
+    // NOTE) we should clone functype here because functype needs to be maintained for host function call later.
     own wasm_functype_t* functypeCopy = wasm_functype_copy(functype);
 
     WASMHostFunctionEnvironment* env = new WASMHostFunctionEnvironment(func, functypeCopy);
-    wasm_func_t* funcaddr = wasm_func_new_with_env(state.context()->vmInstance()->wasmStore(), functypeCopy, callbackHostFunction, env, nullptr);
+    own wasm_func_t* funcaddr = wasm_func_new_with_env(state.context()->vmInstance()->wasmStore(), functypeCopy, callbackHostFunction, env, nullptr);
 
     state.context()->wasmEnvCache()->push_back(env);
 
@@ -266,6 +266,7 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
 
     // Let imports be << >>
     wasm_extern_vec_new_uninitialized(imports, import_types.size);
+    size_t importsSize = 0;
 
     // For each (moduleName, componentName, externtype) of module_imports(module),
     for (size_t i = 0; i < import_types.size; i++) {
@@ -281,10 +282,7 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
         // If Type(o) is not Object, throw a TypeError exception.
         if (!o.isObject()) {
             wasm_importtype_vec_delete(&import_types);
-            // FIXME should not call destructor of each element in imports
-            // wasm_extern_vec_delete(&imports);
-            delete[] imports->data;
-            imports->size = 0;
+            wasm_extern_vec_delete_with_size(imports, importsSize);
 
             ErrorObject::throwBuiltinError(state, ErrorObject::TypeError, ErrorObject::Messages::WASM_ReadImportsError);
         }
@@ -307,7 +305,8 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
             if (v.asObject()->isExportedFunctionObject()) {
                 // If v has a [[FunctionAddress]] internal slot, and therefore is an Exported Function,
                 // Let funcaddr be the value of v?s [[FunctionAddress]] internal slot.
-                funcaddr = v.asObject()->asExportedFunctionObject()->function();
+                // Note) copy func reference because ExportedFunctionObject has the origin wasm_func_t reference
+                funcaddr = wasm_func_copy(v.asObject()->asExportedFunctionObject()->function());
             } else {
                 // Create a host function from v and functype, and let funcaddr be the result.
                 funcaddr = wasmCreateHostFunction(state, v.asObject(), wasm_externtype_as_functype(externtype));
@@ -343,7 +342,8 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
                 }
 
                 // Let value be ToWebAssemblyValue(v, valtype).
-                wasm_val_t value = WASMValueConverter::wasmToWebAssemblyValue(state, v, wasm_valtype_kind(valtype));
+                // Note) value should not have any reference in itself, so we don't have to call `wasm_val_delete`
+                own wasm_val_t value = WASMValueConverter::wasmToWebAssemblyValue(state, v, wasm_valtype_kind(valtype));
 
                 // Let (store, globaladdr) be global_alloc(store, const valtype, value).
                 // FIXME globaltype
@@ -354,7 +354,8 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
             } else if (v.isObject() && v.asObject()->isWASMGlobalObject()) {
                 // Otherwise, if v implements Global,
                 // Let globaladdr be v.[[Global]].
-                globaladdr = v.asObject()->asWASMGlobalObject()->global();
+                // Note) copy global reference because WASMGlobalObject has the origin wasm_global_t reference
+                globaladdr = wasm_global_copy(v.asObject()->asWASMGlobalObject()->global());
             } else {
                 // Throw a LinkError exception.
                 throwLinkError = true;
@@ -378,7 +379,8 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
 
             // Let externmem be the external value mem v.[[Memory]].
             // Append externmem to imports.
-            wasm_memory_t* externmem = v.asObject()->asWASMMemoryObject()->memory();
+            // Note) copy memory reference because WASMMemoryObject has the origin wasm_memory_t reference
+            wasm_memory_t* externmem = wasm_memory_copy(v.asObject()->asWASMMemoryObject()->memory());
             imports->data[i] = wasm_memory_as_extern(externmem);
             break;
         }
@@ -395,7 +397,8 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
             // Let tableaddr be v.[[Table]].
             // Let externtable be the external value table tableaddr.
             // Append externtable to imports.
-            wasm_table_t* tableaddr = v.asObject()->asWASMTableObject()->table();
+            // Note) copy table reference because WASMTableObject has the origin wasm_table_t reference
+            wasm_table_t* tableaddr = wasm_table_copy(v.asObject()->asWASMTableObject()->table());
             imports->data[i] = wasm_table_as_extern(tableaddr);
             break;
         }
@@ -407,13 +410,12 @@ static void wasmReadImportsOfModule(ExecutionState& state, wasm_module_t* module
 
         if (UNLIKELY(throwLinkError)) {
             wasm_importtype_vec_delete(&import_types);
-            // FIXME should not call destructor of each element in imports
-            // wasm_extern_vec_delete(&imports);
-            delete[] imports->data;
-            imports->size = 0;
+            wasm_extern_vec_delete_with_size(imports, importsSize);
 
             ErrorObject::throwBuiltinError(state, ErrorObject::WASMLinkError, ErrorObject::Messages::WASM_ReadImportsError);
         }
+
+        importsSize++;
     }
 
     wasm_importtype_vec_delete(&import_types);
@@ -527,11 +529,7 @@ static Value wasmInstantiateModule(ExecutionState& state, Value thisValue, size_
     // Instantiate the core of a WebAssembly module module with imports, and let instance be the result.
     own wasm_trap_t* trap = nullptr;
     own wasm_instance_t* instance = wasm_instance_new(state.context()->vmInstance()->wasmStore(), module, imports.data, &trap);
-
-    // FIXME should not call destructor of each element in imports
-    // wasm_extern_vec_delete(&imports);
-    delete[] imports.data;
-    imports.size = 0;
+    wasm_extern_vec_delete(&imports);
 
     // TODO error exception
     if (!instance) {
@@ -566,16 +564,16 @@ static Value wasmInstantiateCoreModule(ExecutionState& state, Value thisValue, s
     WASMModuleObject* moduleObj = argv[0].asPointerValue()->asWASMModuleObject();
     wasm_module_t* module = moduleObj->module();
 
-    const wasm_extern_t** importsData = state.resolveCallee()->asExtendedNativeFunctionObject()->internalSlotAsPointer<const wasm_extern_t*>(0);
-    state.resolveCallee()->asExtendedNativeFunctionObject()->setInternalSlotAsPointer(0, nullptr);
+    ExtendedNativeFunctionObject* callee = state.resolveCallee()->asExtendedNativeFunctionObject();
+    size_t importsSize = callee->internalSlot(0).asUInt32();
+    wasm_extern_t** importsData = callee->internalSlotAsPointer<wasm_extern_t*>(1);
+    own wasm_extern_vec_t imports = { importsSize, importsData };
+    callee->setInternalSlotAsPointer(1, nullptr);
 
     // Instantiate the core of a WebAssembly module module with imports, and let instance be the result.
     own wasm_trap_t* trap = nullptr;
-    own wasm_instance_t* instance = wasm_instance_new(state.context()->vmInstance()->wasmStore(), module, importsData, &trap);
-
-    // FIXME should not call destructor of each element in imports
-    // wasm_extern_vec_delete(&imports);
-    delete[] importsData;
+    own wasm_instance_t* instance = wasm_instance_new(state.context()->vmInstance()->wasmStore(), module, imports.data, &trap);
+    wasm_extern_vec_delete(&imports);
 
     // TODO error exception
     if (!instance) {

src/wasm/WASMObject.cpp

@@ -27,6 +27,10 @@
 #include "wasm/WASMObject.h"
 #include "wasm/WASMValueConverter.h"
 
+// represent ownership of each object
+// object marked with 'own' should be deleted in the current context
+#define own
+
 namespace Escargot {
 
 WASMHostFunctionEnvironment::WASMHostFunctionEnvironment(Object* f, wasm_functype_t* ft)
@@ -321,7 +325,8 @@ Value WASMGlobalObject::getGlobalValue(ExecutionState& state) const
     wasm_global_t* globaladdr = global();
 
     // Let value be global_read(store, globaladdr).
-    wasm_val_t value;
+    // Note) value should not have any reference in itself, so we don't have to call `wasm_val_delete`
+    own wasm_val_t value;
     wasm_global_get(globaladdr, &value);
 
     // Return ToJSValue(value).

third_party/wasm/include/wasm.h

@@ -87,7 +87,9 @@ typedef double float64_t;
     size_t, own wasm_##name##_t ptr_or_none const[]); \
   WASM_API_EXTERN void wasm_##name##_vec_copy( \
     own wasm_##name##_vec_t* out, const wasm_##name##_vec_t*); \
-  WASM_API_EXTERN void wasm_##name##_vec_delete(own wasm_##name##_vec_t*);
+  WASM_API_EXTERN void wasm_##name##_vec_delete(own wasm_##name##_vec_t*); \
+  WASM_API_EXTERN void wasm_##name##_vec_delete_with_size( \
+    own wasm_##name##_vec_t*, size_t);
 
 
 // Byte vectors

tools/test/wasm-js/func_index_patch

@@ -1,25 +0,0 @@
-diff --git src/interp/interp-wasm-c-api.cc src/interp/interp-wasm-c-api.cc
-index 75190feb..8fcc98a8 100644
---- src/interp/interp-wasm-c-api.cc
-+++ src/interp/interp-wasm-c-api.cc
-@@ -760,6 +760,20 @@ void wasm_instance_exports(const wasm_instance_t* instance,
-   }
- }
- 
-+uint32_t wasm_instance_func_index(const wasm_instance_t* instance,
-+                                const wasm_func_t* func) {
-+  auto&& funcs = instance->As<Instance>()->funcs();
-+
-+  assert(func.size() < wasm_limits_max_default);
-+  for (size_t i = 0; i < funcs.size(); ++i) {
-+    if (funcs[i] == func->I.ref()) {
-+      return i;
-+    }
-+  }
-+
-+  return wasm_limits_max_default;
-+}
-+
- // wasm_functype
- 
- own wasm_functype_t* wasm_functype_new(own wasm_valtype_vec_t* params,

tools/test/wasm-js/wabt_patch

@@ -0,0 +1,42 @@
+diff --git src/interp/interp-wasm-c-api.cc src/interp/interp-wasm-c-api.cc
+index 75190feb..867cc607 100644
+--- src/interp/interp-wasm-c-api.cc
++++ src/interp/interp-wasm-c-api.cc
+@@ -760,6 +760,20 @@ void wasm_instance_exports(const wasm_instance_t* instance,
+   }
+ }
+ 
++uint32_t wasm_instance_func_index(const wasm_instance_t* instance,
++                                const wasm_func_t* func) {
++  auto&& funcs = instance->As<Instance>()->funcs();
++
++  assert(func.size() < wasm_limits_max_default);
++  for (size_t i = 0; i < funcs.size(); ++i) {
++    if (funcs[i] == func->I.ref()) {
++      return i;
++    }
++  }
++
++  return wasm_limits_max_default;
++}
++
+ // wasm_functype
+ 
+ own wasm_functype_t* wasm_functype_new(own wasm_valtype_vec_t* params,
+@@ -1170,6 +1184,16 @@ void wasm_val_vec_delete(own wasm_val_vec_t* vec) {
+     }                                                                   \
+     delete[] vec->data;                                                 \
+     vec->size = 0;                                                      \
++  }                                                                     \
++  void wasm_##name##_vec_delete_with_size(wasm_##name##_vec_t* vec,     \
++                                          size_t size) {                \
++    assert(size <= vec->size);                                          \
++    TRACE0();                                                           \
++    for (size_t i = 0; i < size; ++i) {                                 \
++      delete vec->data[i];                                              \
++    }                                                                   \
++    delete[] vec->data;                                                 \
++    vec->size = 0;                                                      \
+   }
+ 
+ WASM_IMPL_VEC_OWN(frame);