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

Fix bug in wasm2c runtime's alternate stack deallocation #2466

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

squk
Copy link
Contributor

@squk squk commented Sep 17, 2024

This makes wasm_rt_init() properly reentrant. By setting g_alt_stack = NULL, we allow execution to continue beyond wasm-rt-impl.c L171 in os_allocate_and_install_altstack.

Applies to debug builds only due to assert.

This makes `wasm_rt_init()` properly reentrant.
@sbc100 sbc100 requested a review from keithw September 18, 2024 15:11
@sbc100
Copy link
Member

sbc100 commented Sep 18, 2024

Seems reasonable to me. I guess technically the resetting could also be in ifndef NDEBUG but perhaps thats overkill?

@squk
Copy link
Contributor Author

squk commented Sep 18, 2024

Yeah I dont think this should be in ifndef NDEBUG, as assigning the pointer to null is the proper cleanup behavior. Regardless of if the failure is triggered in debug or prod builds, the runtime state should be properly maintained in both.

@sbc100
Copy link
Member

sbc100 commented Sep 18, 2024

Yeah I dont think this should be in ifndef NDEBUG, as assigning the pointer to null is the proper cleanup behavior. Regardless of if the failure is triggered in debug or prod builds, the runtime state should be properly maintained in both.

I think "assigning the pointer to null is the proper cleanup behavior" is debatable, but I also don't think its super important in this case.

Taken to the limit I think that would mean that all C++ destructors should zero all their fields. For performance reasons we don't tend to do this kind of thing in release builds.

@shravanrn
Copy link
Collaborator

shravanrn commented Sep 18, 2024

Agreed, that this would be needed in this case due to re-entrancy (this is independent of discussions around hardening destructors), i.e., calling os_allocate_and_install_altstack, os_disable_and_deallocate_altstack, os_allocate_and_install_altstack in sequence should succeed.

@squk
Copy link
Contributor Author

squk commented Sep 20, 2024

Thanks for the reviews yall!

@sbc100 sbc100 merged commit b287470 into WebAssembly:main Sep 20, 2024
18 checks passed
@squk squk deleted the patch-1 branch September 23, 2024 19:53
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.

3 participants