-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-29640: Protect key list during fork() #783
Conversation
@the-knights-who-say-ni do you have an ETA to merge this commit |
I have an issue with this patch. |
If |
@nicopauss, just for reference, could you share your use case for |
pid = fork(); | ||
#ifdef WITH_THREAD | ||
_PyThread_ReleaseKeyLock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in the child process, I don't think that it's safe to use keymutex just after fork.
A few lines below, PyOS_AfterFork() is called if we are in the child process, this function calls PyThread_ReInitTLS():
void
PyThread_ReInitTLS(void)
{
...
/* As with interpreter_lock in PyEval_ReInitThreads()
we just create a new lock without freeing the old one */
keymutex = PyThread_allocate_lock();
...
}
So keymutex is replaced in the child process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a compromise would be only unlock in the parent process. _PyThread_ReleaseKeyLock(); in the parent is safe, whereas _PyThread_ReleaseKeyLock(); in the child can be simply... ignored, since we replace keymutex a few instructions later (see my previous comment).
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@@ -3869,7 +3880,18 @@ posix_fork(PyObject *self, PyObject *noargs) | |||
pid_t pid; | |||
int result = 0; | |||
_PyImport_AcquireLock(); | |||
#ifdef WITH_THREAD | |||
if (_PyThread_AcquireKeyLock() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a comment explaining the rationale of this lock. I understand that it's a workaround to make the Python TLS implementation "fork-safe".
pid = fork(); | ||
#ifdef WITH_THREAD | ||
_PyThread_ReleaseKeyLock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a compromise would be only unlock in the parent process. _PyThread_ReleaseKeyLock(); in the parent is safe, whereas _PyThread_ReleaseKeyLock(); in the child can be simply... ignored, since we replace keymutex a few instructions later (see my previous comment).
I changed the PR title to make @bedevere-bot happy (to fix bedevere/issue-number test). |
_PyThread_AcquireKeyLock(void) | ||
{ | ||
if (keymutex == NULL) { | ||
keymutex = PyThread_allocate_lock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a special case if the mutex wasn't created yet: if keymutex is NULL, well, just do nothing, no?
I guess that that the keymutex variable is protected by the GIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could just be assert(keymutex != NULL);
.
keymutex
is first initialized from Py_Initialize
→ _PyGILState_Init
.
See #5141 for a more comprehensive (but still unfinished) solution. |
When a thread is modifying the key list and a fork() happens in another thread, the forked process can be left with an inconsistent key list. More info at: http://bugs.python.org/issue29640
This PR adds two new functions, _PyThread_AcquireKeyLock and _PyThread_ReleaseKeyLock, which are used to lock the key mutex around fork() calls.
https://bugs.python.org/issue29640