Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure that touching pages will not fail due to misconfigured protection #4067

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

loganek
Copy link
Collaborator

@loganek loganek commented Feb 6, 2025

When threads are initialized with wasm_runtime_init_thread_env(), the first few pages (configured using STACK_OVERFLOW_CHECK_GUARD_PAGE_COUNT) are configured to be not readable/writable (for the stack protection). The protection is then removed in destroy_stack_guard_pages(), which is automatically called when os_thread_wrapper is used for creating a new thread.
However, the wrapper might not always be used (and in fact, is not in some of the examples like here) when the thread is created outside of WAMR. To avoid crashes in touch_pages (caused by trying to write to protected memory), we'll mark the whole stack readable/writable before touching the pages.

…ection

When threads are initialized with `wasm_runtime_init_thread_env()`, the first
few pages (configured using `STACK_OVERFLOW_CHECK_GUARD_PAGE_COUNT`) are
configured to be not readable/writable (for the stack protection).
The protection is then removed in `destroy_stack_guard_pages()`, which is
automatically called when `os_thread_wrapper` is used for creating a new
thread.
However, the wrapper might not always be used (and in fact, is not in some
of the examples like [here](samples/terminate/src/main.c)) when the thread
is created outside of WAMR. To avoid crashes, we'll mark the whole stack
readable/writable before touching the pages.
@lum1n0us
Copy link
Collaborator

lum1n0us commented Feb 7, 2025

caused by trying to write to protected memory

Not quite following which line is intended to write to protected pages.

if using the code mentioned above, it seems that init_stack_guard_pages() are paired with destroy_stack_guard_pages().

static void *
runner_with_sigleton_exec_env(void *vp)
{
    wasm_module_inst_t inst = vp;
    bool ok = wasm_runtime_init_thread_env();  //->init_stack_guard_pages()
    assert(ok);
    wasm_application_execute_main(inst, 0, NULL);
    wasm_runtime_destroy_thread_env();  //->destroy_stack_guard_pages()
    return inst;
}

@loganek
Copy link
Collaborator Author

loganek commented Feb 7, 2025

oh you're right, in the example above indeed there's a call to destroy_stack_guard_pages, sorry I missed it. Looks like making both calls is a requirement then, and it cannot be relaxed? It might be a bit problematic though in case the lifecycle of the thread isn't that clear in a more complex environments (in our case a 3rd party might choose to spawn a thread and call a callback provided by our application; the 3rd party doesn't know what's in the callback, therefore doesn't know anything about the existence of WAMR).

@lum1n0us
Copy link
Collaborator

Perhaps we could either have 3rd parties use an Amazon-defined start_routine() and employ a callback to execute 3rd party code, or use something akin to pthread_cleanup_xxx()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants