bytecodealliance
[RFC] Explicitly validating the pointers against null
Avoid null pointer de-reference errors by explicitly validating the pointers against null before using the pointer
A Well-known rules of defensive programming. But does wamr need it? I knew we're applying caller guarantee rules. And it's a simple checker (but a lot).
There are 5 files in core/iwasm/includes and approximately 170 APIs in wasm_export.h, 1 API in lib_export.h, 15 APIs in aot_export.h, 65 APIs in gc_export.h, and 180 APIs in wasm_c_api.h.
Additionally, char* should be considered as a pointer first before being treated as a string.
Furthermore, a return value is required after implementing pointer parameter checking. Approximately 80 APIs might need redesigning because they previously returned void.
A basic 💡 is:
Develop a new set of APIs, named something like xxx_checked, based on existing APIs. like wasm_runtime_get_export_type_checked(), wasm_runtime_attach_shared_heap_checked() and so on.
Each new API will act as a wrapper for its corresponding existing API. like
#include "wasm_export.h"
static inline bool
wasm_runtime_attach_shared_heap_checked(wasm_module_inst_t module_inst,
wasm_shared_heap_t shared_heap)
{
CHECK_NULL_OR_RET_FALSE(module_inst);
CHECK_NULL_OR_RET_FALSE(shared_heap);
return wasm_runtime_attach_shared_heap(module_inst, shared_heap);
}
All new APIs will be placed in new files, named something like xxx_checked. like wasm_export_checked.h, lib_export_checked.h and so on.
Can we embed it in exisitng API and control it via build flags, which is similar to fortity?
Fortify is a suite of tools used for application security testing, which analyzes software code to find and fix security vulnerabilities before deployment.
Do you mean this fortify?
Can we embed it in existing API and control it via build flags?
@no1wudi I'm not following. Would you mind giving an example?
honestly speaking, i feel this is too much complexity for little or no benefits.
as it's to detect programming errors, it's more appropriate to use assertions.
bh_assert(module_inst != NULL);
as it's to detect programming errors, it's more appropriate to use assertions.
I think the new checked version also needs to return gracefully instead of aborting from an assertion.
honestly speaking, i feel this is too much complexity for little or no benefits.
💯Yes, this is used to protect APIs from unpredictable incoming parameters. It won't change our "caller guarantee" rules mentioned in the security issues definition. This is partially why I believe the checked version should be kept separately, as it can provide users with a hint about parameter checking.
This is the implantation. https://github.com/bytecodealliance/wasm-micro-runtime/pull/4682
as it's to detect programming errors, it's more appropriate to use assertions.
I think the new checked version also needs to return gracefully instead of aborting from an assertion.
why?
i bet that someone who passes us NULL when he shouldn't doesn't check returned errors. it would be more helpful to abort. we can even use a special version of bh_assert, which complain on the user error loudly.
In my mind, these are the differences between "caller guarantee" and "callee guarantee." In a caller guarantee, the caller should always check parameters before calling. In a callee guarantee, the caller should always check the return value after calling. Otherwise, the caller can't ensure that the call is working well, and the caller should always verify something if quality is required.
Providing two sets of APIs for the caller's choice is not a bad idea, at least. It might not be very beneficial, which is why I finally implemented a PR.
Aborting might not be a good idea:
- A producer might hope the API always returns gracefully and has the chance to capture errors through their own system (if they have one). Aborting in a release build may not be expected.
- Using abortion only in a debugging build might lead to a situation where developers have to reproduce some issues in a debug build again. We all know some issues are very hard to reproduce.
I agree we should complain about the user error loudly, so maybe using something like snprintf() to log?
@yamt hope to receive your opinion.
In my mind, these are the differences between "caller guarantee" and "callee guarantee." In a caller guarantee, the caller should always check parameters before calling. In a callee guarantee, the caller should always check the return value after calling. Otherwise, the caller can't ensure that the call is working well, and the caller should always verify something if quality is required.
well, it seems you are assuming that it's possible for the callee to provide some useful guarantee. actually it's mostly impossible. for example, consider the caller passes us (void*)1.
Providing two sets of APIs for the caller's choice is not a bad idea, at least. It might not be very beneficial, which is why I finally implemented a PR.
IMO, it's a bad idea because it effectively doubles the size of the api surface to test/maintain.
Aborting might not be a good idea:
* A producer might hope the API always returns gracefully and has the chance to capture errors through their own system (if they have one). Aborting in a release build may not be expected. * Using abortion only in a debugging build might lead to a situation where developers have to reproduce some issues in a debug build again. We all know some issues are very hard to reproduce.
i can't imagine the case where
- a user wants to handle the case gracefully
- AND the user doesn't want to check NULL by themselves before passing it to us
can you elaborate the use case a bit?
I agree we should complain about the user error loudly, so maybe using something like snprintf() to log?
@yamt hope to receive your opinion.
fuzzing ...
And the key point is to prevent the runtime from crashing by dereferencing a NULL pointer.
if an application is passing us NULL where it shouldn't, the application is broken and it will likely crash sooner or later anyway. IMO, the only thing we can do is to tell the application developer it's broken so that he can hopefully fix the application. bh_assert (or its variation) is better than returning an error for the purpose.
Let me summarize:
- We don't mind adding input parameter validation, and we do not want to maintain two sets of APIs.
- The disagreement we have is whether to use the if+error code method or the assertion.
Assertions can indeed draw the attention of API users and provide enough motivation to make changes. However, there are concerns about using assertions:
- Assertions are typically disabled in release versions, so they cannot provide parameter protection in those versions.
- If assertions are enabled in release versions, they might cause the service to stop due to incorrect input parameters (DDOS), potentially escalating a "recoverable error" to an "unrecoverable error."
Here is a compromise solution:
#ifdef NDEBUG
define CHECK(cond) \
do { if (!(cond)) return ERR_INVARIANT; } while(0)
#else
define CHECK(cond) assert(cond)
#endif