Add runtime inited checks in Enclave command handlings to improve security

[wenyongh]

As reported in #2252 (comment),
wait_map may be referred again after free in linux-sgx ecall command if it isn't set NULL.

[yamt]

are you trying to make wasm_runtime_destroy idempotent?
otherwise, i guess it should be fixed in Enclave.cpp instead.

[wenyongh]

are you trying to make wasm_runtime_destroy idempotent? otherwise, i guess it should be fixed in Enclave.cpp instead.

Good point, I added more checks in Enclave instead to fix some similar issues.

[yamt]

lgtm

[LeoneChen]

Hello, I think wait_map = NULL; is necessary. And I found other functions use wait_map already have check wait_map is NULL at function entrypoint like bh_hash_map_find, then this problem can be fixed.

If you only fix at linux-sgx platform, other platform may also face same UAF bug

[yamt]

Hello, I think wait_map = NULL; is necessary. And I found other functions use wait_map already have check wait_map is NULL at function entrypoint like bh_hash_map_find, then this problem can be fixed.

If you only fix at linux-sgx platform, other platform may also face same UAF bug

the bug (calling wasm_runtime_destroy twice) was in linux-sgx and it has been fixed.

if other platforms have similar bugs, they should be fixed too. (have they?)

having double-free checks everywhere is not viable, IMO.

[LeoneChen]

It's different from free since the caller needs to specify which pointer to free, e.g. free(ptr), it's the caller's duty to set ptr to null.

However, in WAMR, API is wasm_runtime_destroy() and it free wait_map internal, caller (library user) can't access the wait_map variable, then it's duty of wasm_runtime_destroy() to set wait_map to NULL

I think it's hard to ensure we don't have double free in other platform

[yamt]

It's different from free since the caller needs to specify which pointer to free, e.g. free(ptr), it's the caller's duty to set ptr to null.

However, in WAMR, API is wasm_runtime_destroy() and it free wait_map internal, caller (library user) can't access the wait_map variable, then it's duty of wasm_runtime_destroy() to set wait_map to NULL

i disagree. while it's ok to set wait_map to NULL, it's far from a "duty".
calling wasm_runtime_destroy twice as sgx was doing is an undefined behavior as double free is.
what you are saying is something like it's free's duty to prepare to handle double free.

[LeoneChen]

All right