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 Unix.create_process_env thread-safe in all cases #12404

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

xavierleroy
Copy link
Contributor

If neither spawnp nor execvpe are provided by the C library, the emulation code in otherlibs/unix, which is based on fork, ended up allocating memory in the child process using malloc.

This is not thread-safe: if the fork occurs while another OCaml domain or C thread is running inside malloc, the child process can inherit a memory state where it's unsafe to do malloc. For example, the mutex protecting malloc is locked, because it was locked at fork time, but the other thread is no longer there to finish the malloc and unlock the mutex, so malloc deadlocks in the child process.

This PR proposes a simple fix: in the child process, instead of calling our malloc-based emulation of execvpe, we just set environ to the desired environment and call execvp. This is safe because no other thread is running in the child process, so nobody can observe the change to the global variable environ.

Fixes: #12395.

@xavierleroy
Copy link
Contributor Author

I did a round of precheck after disabling the posix_spawn path, so that our emulation is called on all platforms, and the results look good.

Copy link
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat trick! LGTM.

If neither `spawnp` nor `execvpe` are provided, the emulation code ended up allocating memory in the child process using `malloc`, which is not thread-safe.

As a fix, in the child process we just set `environ` and call `execvp`.  This is safe because no other thread is running in the child process.
@xavierleroy xavierleroy merged commit 96d1702 into ocaml:trunk Aug 7, 2023
9 checks passed
@xavierleroy xavierleroy deleted the create_process_env branch August 7, 2023 08:01
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.

Unix.create_process_env might not be multi-thread safe
2 participants