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

Deadlock in emscripten dynamic calls #2769

Merged
merged 3 commits into from
Jan 27, 2022
Merged

Conversation

jcaesar
Copy link
Contributor

@jcaesar jcaesar commented Jan 27, 2022

Description

Attempting to run an emscripten-built rust module lead me to a deadlock. I'm not quite sure what's wrong, but it looks to me like emscripten is calling itself through the host (No idea why. To catch exceptions?) and in the process, wasmer acquires a lock that is still held when calling back into the module. If the module tries that twice recursively, the simplest form of deadlock, a reentrancy deadlock is triggered.

The standard library doesn't have reentrant locks.
This cannot be solved by replacing the lock on EmEnv.data by an RwLock, because the code might also call setTempRet0, which does require a write lock.
Maybe there is a way to get rid of the mutex entirely, but I do not see it.

The easy way out seems to be to clone the function (arcs), let go of the lock, and then do the call back into the module.

There are a few other instances of the same problematic pattern, but they call memset or malloc, which shouldn't call back into wasmer. I assume they are fine and left them alone, but have not done thorough testing.

Review

  • Add a short description of the change to the CHANGELOG.md file

Do not hold locks on EmEnv's EmscriptenData into the actual call into the module
@Amanieu
Copy link
Contributor

Amanieu commented Jan 27, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

@bors bors bot merged commit 42059f7 into wasmerio:master Jan 27, 2022
@jcaesar jcaesar deleted the emscripten-deadlock branch January 28, 2022 01:31
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