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

Protection against fork #735

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

Protection against fork #735

wants to merge 1 commit into from

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Jan 24, 2025

If a thread forks, while another thread is holding an snmalloc lock, then the allocator could stop working.

This patch attempts to protect against the cases of this. There is one case that is not covered. If a fork occurs during the very first allocation. This can result in the installation of the fork handler racing with the fork, and all bets are off.

Address #630

If a thread forks, while another thread is holding an snmalloc lock, then the allocator could stop working.

This patch attempts to protect against the cases of this. There is one case that is not covered. If a fork occurs during the very first allocation. This can result in the installation of the fork handler racing with the fork, and all bets are off.
@SchrodingerZhu
Copy link
Collaborator

Looking into it today

@SchrodingerZhu
Copy link
Collaborator

Need to adjust header order.

The change looks good to me but I may need to double check whether snmalloc is calling any non-async-signal-safe function (which would be UB if used after fork).

@davidchisnall
Copy link
Collaborator

Calling malloc after fork is undefined behaviour, so I’m not too worried about this as a problem (which is why I didn’t fix it three years ago). Using the atfork hooks may be problematic because the exact time at which they’re called is a bit loosely defined and we’re unordered with respect to other users of these APIs. I am not sure what happens in your code if another user calls malloc or free in their pre/post fork code.

FreeBSD libc provides two hooks: _malloc_prefork and _malloc_postfork, which are guaranteed to be called only in multithreaded code and called after other prefork before other post-fork hooks. Acquiring the lock in the prefork and releasing it in postfork should be sufficient there.

These hooks exist because jemalloc determined that malloc being fork safe was not possible with the standard pthread APIs.

We should probably expose this as a PAL hook as we do with some of the other libc-specific things.

@mjp41
Copy link
Member Author

mjp41 commented Jan 25, 2025

@davidchisnall, I just want to check. The original issue that you raised said:

ideally malloc between fork and execve should work

But I think from your more recent message on this PR, this is UB, and not something we should address.

I am not sure what happens in your code if another user calls malloc or free in their pre/post fork code.

This is a great observation, I hadn't consider multiple handlers. This code could deadlock if an allocation occurs in a later prefork, or earlier postfork than the ones supplied by snmalloc. So this code makes things worse, than doing nothing.

I am going to drop this PR, and close the underlying issue as I believe we are as safe as the standard requires.

@SchrodingerZhu
Copy link
Collaborator

SchrodingerZhu commented Jan 26, 2025

FYI, according to POSIX.1-2024,

A process shall be created with a single thread. If a multi-threaded process calls fork(), the new process shall contain a replica of the calling thread and its entire address space, possibly including the states of mutexes and other resources. Consequently, the application shall ensure that the child process only executes async-signal-safe operations until such time as one of the exec functions is successful.

So, only a very limited list of posix functions could be invoked post fork. ( I think direct syscall wrappers that are not in POSIX are also safe to call).

However, fork is like an "grey area" for many libc implementations, especially when considering threaded envrionment with async signals. glibc, for example, runs into many troubles such as pid caching invalidation across fork (where gettid/getpid may return wrong values in signal frames that triggered right after SYS_fork before post-fork hooks are called. While such issue can be fixed, they finally give up caching tid/pid due to its complexity). AFAIK, some libc implementations themselves are calling non-async-signal-safe functions inside their fork wrappers.

Hence, I think a better idea is not to promise anything to users on our post-fork or signal-frame usability.

@davidchisnall
Copy link
Collaborator

Note, in particular, that malloc and free are not on the list of async signal safe functions, so it’s fine according to POSIX for a multithreaded program’s child to deadlock if it calls snmalloc between fork and exec.

That said, FreeBSD libc does provide hooks that allow the bundled jemalloc to make this work. Snmalloc would have to acquire all of the locks for chunk allocators in the pre hook and release them in the post hook. I don’t think it’s worth trying to make this work on platforms that don’t expose equivalent hooks (I think most libcs do, perhaps with different spellings).

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