core/config.h: Bump the default WASM_STACK_GUARD_SIZE #3344
Conversation
The old value (1KB) doesn't seem sufficient for many cases. I suspect that the new value is still not sufficient for some cases. But it's far safer than the old value. Consider if the classic interpreter loop (2600 bytes) calls host snprintf. (2000 bytes) Fixes: bytecodealliance#3314
| @@ -486,7 +486,7 @@ | |||
| /* UVWASI requires larger native stack */ | |||
| #define WASM_STACK_GUARD_SIZE (4096 * 6) | |||
| #else | |||
| #define WASM_STACK_GUARD_SIZE (1024) | |||
| #define WASM_STACK_GUARD_SIZE (1024 * 5) | |||
There was a problem hiding this comment.
Is it better to increase the size only for linux/macos or posix like platforms? Seems that not all platforms need so much guard stack space, e.g. zephyr/esp-idf 32-bit, had better keep it unchanged for them now?
There was a problem hiding this comment.
maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?
There was a problem hiding this comment.
BTW, maybe we can also decrease the native stack usage by refining the code which calls snprintf? For example, in wasm_set_exception_local and set_exception_visitor, copy character one by one from source string until '\0' is found.
There was a problem hiding this comment.
maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?
I am not sure, my suggestion is to keep it unchanged temporarily, and change it after we do more test.
There was a problem hiding this comment.
ps. keep it unchanged for non-posix like platforms until we do more test.
There was a problem hiding this comment.
maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?
I am not sure, my suggestion is to keep it unchanged temporarily, and change it after we do more test.
i can probably investigate the situation on xtensa/nuttx. do you think it's good enough to make it represent the "low end" side?
OK, thanks. Distinguish it into high end side and low end side is good to me, do yo think we should put the macro in each platform's platform_internal.h?
maybe. after all, it's very platform dependent.
otoh, i suspect that embedders for very small devices usually fine-tune WASM_STACK_GUARD_SIZE for their specific applications anyway.
There was a problem hiding this comment.
BTW, maybe we can also decrease the native stack usage by refining the code which calls snprintf? For example, in wasm_set_exception_local and set_exception_visitor, copy character one by one from source string until '\0' is found.
i agree it's probably a good idea to reduce the stack usage of wasm_runtime_set_exception and friends.
but i'm not sure if we can reduce WASM_STACK_GUARD_SIZE with it unless we introduce a rather strong policy about where os_printf, LOG_DEBUG, etc can be used.
Yes, I think we can increase WASM_STACK_GUARD_SIZE to 5KB and also refine the code in some places to enhance the security. And decrease WASM_STACK_GUARD_SIZE only when we do a full test to make sure it is good.
There was a problem hiding this comment.
maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?
I am not sure, my suggestion is to keep it unchanged temporarily, and change it after we do more test.
i can probably investigate the situation on xtensa/nuttx. do you think it's good enough to make it represent the "low end" side?
OK, thanks. Distinguish it into high end side and low end side is good to me, do yo think we should put the macro in each platform's platform_internal.h?
maybe. after all, it's very platform dependent. otoh, i suspect that embedders for very small devices usually fine-tune WASM_STACK_GUARD_SIZE for their specific applications anyway.
OK, so had better keep putting the macro in core/config.h, and maybe we can add a document for detailed description about this macro and the native stack overflow check.
There was a problem hiding this comment.
reverted for non-posix platforms for now.
There was a problem hiding this comment.
maybe. after all, it's very platform dependent. otoh, i suspect that embedders for very small devices usually fine-tune WASM_STACK_GUARD_SIZE for their specific applications anyway.
Agree.
Also, add comments.
…ce#3344) The old value (1KB) doesn't seem sufficient for many cases. I suspect that the new value is still not sufficient for some cases. But it's far safer than the old value. Consider if the classic interpreter loop (2600 bytes) calls host snprintf. (2000 bytes) Fixes: bytecodealliance#3314
…ce#3344) The old value (1KB) doesn't seem sufficient for many cases. I suspect that the new value is still not sufficient for some cases. But it's far safer than the old value. Consider if the classic interpreter loop (2600 bytes) calls host snprintf. (2000 bytes) Fixes: bytecodealliance#3314 Signed-off-by: victoryang00 <victoryang00@ucsc.edu>
…ce#3344) The old value (1KB) doesn't seem sufficient for many cases. I suspect that the new value is still not sufficient for some cases. But it's far safer than the old value. Consider if the classic interpreter loop (2600 bytes) calls host snprintf. (2000 bytes) Fixes: bytecodealliance#3314
…ce#3344) The old value (1KB) doesn't seem sufficient for many cases. I suspect that the new value is still not sufficient for some cases. But it's far safer than the old value. Consider if the classic interpreter loop (2600 bytes) calls host snprintf. (2000 bytes) Fixes: bytecodealliance#3314 Signed-off-by: victoryang00 <victoryang00@ucsc.edu>
The old value (1KB) doesn't seem sufficient for many cases.
I suspect that the new value is still not sufficient for some cases. But it's far safer than the old value.
Consider if the classic interpreter loop (2600 bytes) calls host snprintf. (2000 bytes)
Fixes: #3314