Add a check about WASI ABI compatibility #913

lum1n0us · 2021-12-23T06:54:49Z

Refer to:
https://github.com/WebAssembly/WASI/blob/main/design/application-abi.md

Plus,

refactoring wasm_loader_find_export
remove MULTI_MODULE related from mini_loader
Update the sample of multi-modules
Fix a "use-after-free" problem. Since we are reusing the memory instance of sub-module's,
just to pretect from freeing an imported memory instance is enough

wenyongh · 2021-12-24T10:00:59Z

core/iwasm/common/wasm_application.c

+                                                          NULL);
+    }
+    else {
+        wasm_runtime_set_exception(


Should not set exception here: (1) Must the wasm app compiled as wasi mode export _start function? (2) Here the WASM_ENABLE_LIBC_WASI is enabled, but it doesn't require that the wasm app must be a wasi mode app, the app can be non-wasi mode also, for example /opt/wasi-sdk/bin/clang -nostdlib --no-entry --export=__main_argc_argv ...

If with WASI, a .wasm is either a command or a reactor. There should be not a third case. So. YES we run the WASM app if there is a _start. And we quit with a message if there is no _start.

Plus, we shall treat -nostdlib as a behavior of disabled WASI.

wenyongh · 2021-12-24T10:25:43Z

core/iwasm/common/wasm_runtime_common.c


 WASMFunctionInstanceCommon *
-wasm_runtime_lookup_function(WASMModuleInstanceCommon *const module_inst,
+wasm_runtime_lookup_function(WASMModuleInstanceCommon *module_inst,


Shall we not change the API's prototype? I had a test, it doesn't cause compile error or warning. Here using const is reasonable, and the API had been used for a long time.

Could we not change the function prototype?

wenyongh · 2021-12-24T10:30:11Z

core/iwasm/common/wasm_application.c

+    }
 #else
-    const char *function_name = name;
+    function_name = name;


Compile warning occurs: assignment discards ‘const’ qualifier from pointer target type

wenyongh · 2021-12-24T10:38:50Z

core/iwasm/interpreter/wasm_loader.c

+            wasm_loader_find_export(module, "", "_initialize", EXPORT_KIND_FUNC,
+                                    error_buf, error_buf_size);
+        if (!initialize) {
+            set_error_buf(


The document mentions that A reactor may export a function named _initialize.. If an _initialize export is present, my understanding is that _initialize function can be exported and also be unexported?

A reactor always has an exported _initialize. Hasn't find any exception.

OK. And should we also check that the sub-module should not export _start func, and main-module should not export _initialize func?

Not sure about that. Clang should make sure that _start and _initialize can not be exported at the same time. But it is OK if we want to be safe first. I am open to both.

Plus, if needs a check about signatures of _start and _initialize, we should have it done here.

wenyongh · 2021-12-24T10:47:30Z

core/iwasm/interpreter/wasm_runtime.c

+             */
+            WASMFunctionInstance *initialize =
+                wasm_lookup_function(sub_module_inst, "_initialize", NULL);
+            /* a strong restricttion that a reactor must export a _initialize


Same as above, seems that it doesn't require that _initialize function must be exported.

And we should not use macro WASM_ENABLE_LIBC_WASI to check that the wasm app is in wasi mode or not, normally we should use module->is_wasi_mode

Well, it is what I am worried about. is_wasi_mode doesn't always equal to WASM_ENABLE_LIBC_WASI=1. It represents a module depends on WASI APIs. If a reactor compiled by WASI_SDK and doesn't include any standard libraries, it will not have any imported entries but still a reactor.

void foo() {}

Yes, it is still a reactor and exports _inititialize, but the function does nothing, whehter to call it or not seems meaningless. There ight be two choices: (1) Call _initialize it is found, (2) Call _initialize when it is found and the module is wasi-module.

And also we should check the func signature of _start and _initialize, they must be "()".

I tend to make it simple without considering .wasm is import any wasi_snapshot_preview1 function or not.

wenyongh · 2021-12-24T10:51:01Z

product-mini/platforms/linux-sgx/enclave-sample/Makefile

 Enclave_C_Flags := $(SGX_COMMON_CFLAGS) -nostdinc -fvisibility=hidden -fpie -fstack-protector $(Enclave_Include_Paths)
 ifeq ($(SPEC_TEST), 1)
 	Enclave_C_Flags += -DWASM_ENABLE_SPEC_TEST=1
+	Enclave_C_Flags += -DSGX_DISABLE_WASI


Don't disable WASI by default, SGX supports WASI and enables WASI by default.

Since SPEC test cases are all from WAST and can't be compiled with wasi-sdk-clang and they are not using any WASI apis. we'd better run spec test cases by avoiding WASI module rules.

That's because that we didn't check module->is_wasi_mode when calling _start and _initialize. My understanding is that we check the wasi module rules only when the module is wasi moduel, or many non wasi cases may run failed.

The point here is we should run SPEC-TEST in non-wasi-mode. the modification won't change the default setting of SGX.

wenyongh · 2021-12-24T10:53:12Z

tests/wamr-test-suites/test_wamr.sh

    local EXTRA_COMPILE_FLAGS=""
    # default enabled features
    EXTRA_COMPILE_FLAGS+=" -DWAMR_BUILD_BULK_MEMORY=1"
+    EXTRA_COMPILE_FLAGS+=" -DWAMR_BUILD_LIBC_WASI=0"


Had better not disable wasi by default

wenyongh · 2021-12-25T02:03:46Z

core/iwasm/interpreter/wasm_loader.c

+            wasm_loader_find_export(module, "", "_initialize", EXPORT_KIND_FUNC,
+                                    error_buf, error_buf_size);
+        if (!initialize) {
+            set_error_buf(


OK. And should we also check that the sub-module should not export _start func, and main-module should not export _initialize func?

wenyongh · 2021-12-25T02:08:56Z

core/iwasm/interpreter/wasm_loader.c


+#if (WASM_ENABLE_MULTI_MODULE != 0) && (WASM_ENABLE_LIBC_WASI != 0)
+    /* do a check about WASI Application ABI */
+    if (!check_wasi_abi_compatibility(module, main_module, error_buf,


Only check the compatibility when module is in wasi mode, or error might occur when it is non-wasi module (e.g. compile with /opt/wasi-sdk/bin/clang -nostdlib):

if (module->is_wasi_mode && !check_wasi_abi_compatibility(module, main_module, error_buf,

We shall treat clang -nostdlib equal to WASM_ENABLE_LIBC_WASI=0. I mean we don't accept the third kind of .wasm beyond command/reactor model when `WASM_ENABLE_LIBC_WASI=1.

I guess the initial idea with using -nostdlib is to avoid defining main() and to land our builtin libc feature. since WASI has the command/reactor solution, the workaround for not defining main() should be removed.

LIBC_WASI and LIBC_BUILTIN can be enabled at the same time.

wenyongh · 2021-12-25T02:19:59Z

core/iwasm/interpreter/wasm_runtime.c

+             */
+            WASMFunctionInstance *initialize =
+                wasm_lookup_function(sub_module_inst, "_initialize", NULL);
+            /* a strong restricttion that a reactor must export a _initialize


Yes, it is still a reactor and exports _inititialize, but the function does nothing, whehter to call it or not seems meaningless. There ight be two choices: (1) Call _initialize it is found, (2) Call _initialize when it is found and the module is wasi-module.

And also we should check the func signature of _start and _initialize, they must be "()".

wenyongh · 2021-12-25T02:23:54Z

product-mini/platforms/linux-sgx/enclave-sample/Makefile

 Enclave_C_Flags := $(SGX_COMMON_CFLAGS) -nostdinc -fvisibility=hidden -fpie -fstack-protector $(Enclave_Include_Paths)
 ifeq ($(SPEC_TEST), 1)
 	Enclave_C_Flags += -DWASM_ENABLE_SPEC_TEST=1
+	Enclave_C_Flags += -DSGX_DISABLE_WASI


That's because that we didn't check module->is_wasi_mode when calling _start and _initialize. My understanding is that we check the wasi module rules only when the module is wasi moduel, or many non wasi cases may run failed.

wenyongh · 2021-12-27T11:57:17Z

core/iwasm/interpreter/wasm_loader.c

+        start = wasm_loader_find_export(module, "", "_start", EXPORT_KIND_FUNC,
+                                        error_buf, error_buf_size);
+        if (!start) {
+            /* without _start(), it maybe a reactor which is run via "iwasm -f
+             * XXX reactor" */
+            LOG_VERBOSE("A command should export _start function by default");
+        }
+    }
+    else {
+        initialize =
+            wasm_loader_find_export(module, "", "_initialize", EXPORT_KIND_FUNC,
+                                    error_buf, error_buf_size);


We have implemented API wasm_runtime_lookup_wasi_start_function() in wasm_runtime_common.h/c, could we implement another API e.g. wasm_runtime_lookup_wasi_initialize_function, and here call them instead?
BTW, in wasm_runtime_lookup_wasi_start_function, we check whether the signature of _start func found is "()", can we also check whether the signature of _initialize is also "()"? Or just adding signature checks here.

wasm_runtime_lookup_wasi_start_function requires a module instance and we don't have it yet.

Plan to have a type-check during loading.

wenyongh · 2021-12-27T12:00:20Z

core/iwasm/interpreter/wasm_runtime.c

+             * in a reactor
+             */
+            WASMFunctionInstance *initialize =
+                wasm_lookup_function(sub_module_inst, "_initialize", NULL);


Seems had better implement API wasm_runtime_lookup_wasi_initialize_function and check the signature

after the type check during loading, we can avoid all secondary check.

wenyongh · 2021-12-27T12:03:56Z

core/iwasm/common/wasm_application.c

-        wasm_runtime_set_exception(module_inst, "lookup main function failed");
+        wasm_runtime_set_exception(
+            module_inst,
+            "lookup the entry point symbol(like _start, _main, _main) failed");


How about "lookup the entry point symbol (e.g. main, _main, __main_argc_argv) failed", _start isn't to be find here, and "_main, _main" is duplicated

plan to include the else case of L99 missing the _start? maybe we shall create two messages when

WASM_EABLE_LIBC_WASI=1. lookup the entry point symbol (like _start, main, _main, __main_argc_argv) failed

WAMS_ENABLE_LIBC_WASI=0. lookup the entry point symbol (like main, _main, __main_argc_argv) failed

wenyongh · 2021-12-27T12:09:52Z

core/iwasm/common/wasm_runtime_common.c


 WASMFunctionInstanceCommon *
-wasm_runtime_lookup_function(WASMModuleInstanceCommon *const module_inst,
+wasm_runtime_lookup_function(WASMModuleInstanceCommon *module_inst,


Could we not change the function prototype?

Refer to: https://github.com/WebAssembly/WASI/blob/main/design/application-abi.md Plus, - refactoring wasm_loader_find_export - remove MULTI_MODULE related from mini_loader - Update the sample of multi-modules - Fix a "use-after-free" problem. Since we are reusing the memory instance of sub-module's, just to pretect from freeing an imported memory instance is enough

wenyongh · 2021-12-28T05:37:06Z

core/iwasm/include/wasm_export.h

 */
 WASM_RUNTIME_API_EXTERN wasm_function_inst_t
-wasm_runtime_lookup_function(wasm_module_inst_t const module_inst,
+wasm_runtime_lookup_function(wasm_module_inst_t module_inst,


Should restore const

wenyongh · 2021-12-28T05:39:15Z

core/iwasm/interpreter/wasm_loader.c

+     *
+     * observations:
+     * - clang always injecting either `_start` into a command
+     * - clang always injecting either `_initialize` into a reactor


how about injects?

wenyongh · 2021-12-28T05:47:40Z

core/iwasm/interpreter/wasm_loader.c

+    }
+
+    /* should have one at least */
+    if (module->is_wasi_module && !start && !initialize) {


No sure whether ther rust wasi main module exports _start or not, could you check the standalone case test-rust1?

wenyongh · 2021-12-28T05:56:19Z

core/iwasm/interpreter/wasm_loader.c

+
+    memory = wasm_loader_find_export(module, "", "memory", EXPORT_KIND_MEMORY,
+                                     error_buf, error_buf_size);
+    if (!memory) {


should be is_wasi_module && !memory, non-wasi module doesn't need to export memory

non-wasi modules are supposed to be all filtered out in L3527

it only filters out non wasi module which doesn't export start and initialize
L3532 checks is_wasi_module again

L3532 and L3545 are kinds of protection to make sure that from L3552, there are all cases about WASI modules.

lum1n0us · 2021-12-28T06:30:49Z

core/iwasm/interpreter/wasm_loader.c

+
+    /* filter out non-wasi compatiable modules */
+    if (!module->is_wasi_module && !start && !initialize) {
+        return true;


wasi compatible modules should have at least one of _start or _initialize. observation 1 and 2.

Refer to https://github.com/WebAssembly/WASI/blob/main/design/application-abi.md to check the WASI ABI compatibility: - Command (main module) may export _start function with signature "()" - Reactor (sub module) may export _initialize function with signature "()" - _start and _initialize can not be exported at the same time - Reactor cannot export _start function - Command and Reactor must export memory And - Rename module->is_wasi_module to module->import_wasi_api - Refactor wasm_loader_find_export() - Remove MULTI_MODULE related codes from mini_loader - Update multi-module samples - Fix a "use-after-free" issue. Since we reuse the memory instance of sub module, just to protect it from freeing an imported memory instance

lum1n0us force-pushed the apply_wasi_abi_to_wamr branch from d1586d8 to e583a71 Compare December 23, 2021 08:51

wenyongh reviewed Dec 24, 2021

View reviewed changes

wenyongh reviewed Dec 25, 2021

View reviewed changes

lum1n0us force-pushed the apply_wasi_abi_to_wamr branch from e583a71 to b4ea10b Compare December 27, 2021 06:20

wenyongh reviewed Dec 27, 2021

View reviewed changes

lum1n0us added 4 commits December 28, 2021 12:19

In SGX mode, disable WASI during the SPEC TEST

0adc713

allow enable both LIBC_WASI and LIBC_BUILTIN

3e09d40

add signature checks on _start and _initialize

906625c

lum1n0us force-pushed the apply_wasi_abi_to_wamr branch from b4ea10b to 906625c Compare December 28, 2021 04:24

update informationn when looking for the entry point failed

01cb646

wenyongh reviewed Dec 28, 2021

View reviewed changes

lum1n0us commented Dec 28, 2021

View reviewed changes

lum1n0us added 2 commits December 28, 2021 14:33

fix typoes in loader

71b4ceb

is_wasi_mode was renamed import_wasi_api

721ce83

wenyongh merged commit 50b6474 into bytecodealliance:main Dec 29, 2021

lum1n0us deleted the apply_wasi_abi_to_wamr branch May 10, 2022 14:41

Add a check about WASI ABI compatibility #913

Add a check about WASI ABI compatibility #913

Uh oh!

Conversation

lum1n0us commented Dec 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lum1n0us Dec 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lum1n0us Dec 26, 2021 •

edited

Loading