gh-116738: Make grp module thread-safe #135434
Conversation
|
Note: I am not sure if there is a CI setup where |
colesbury
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 269b454 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135434%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
@yoney - Can you merge main and fix up the conflicts? |
I rebased and force push but I think this is not the right way, I should have merged the main on top of my branch, I am sorry. |
No worries! |
Lib/test/support/threading_helper.py
Outdated
| @@ -248,3 +248,27 @@ def requires_working_threading(*, module=False): | |||
| raise unittest.SkipTest(msg) | |||
| else: | |||
| return unittest.skipUnless(can_start_thread, msg) | |||
|
|
|||
|
|
|||
| def run_concurrently(worker_func, args, nthreads): | |||
There was a problem hiding this comment.
I think that args=() is the overwhelmingly common case. It should be fine to make it a default. For completeness, let's also add a kwargs parameter. So, the signature should end up looking like this:
# It's fine to use a mutable default here because it's not mutated
def run_concurrently(worker_func, nthreads, args=(), kwargs={}):You also need to document this at https://docs.python.org/3/library/test.html#module-test.support.threading_helper.
There was a problem hiding this comment.
Thank you @ZeroIntensity! I updated run_concurrently()
| @@ -261,6 +277,9 @@ grp_getgrnam_impl(PyObject *module, PyObject *name) | |||
| goto out; | |||
| } | |||
| retval = mkgrent(module, p); | |||
| #ifndef HAVE_GETGRNAM_R | |||
| PyMutex_Unlock(&getgrnam_mutex); | |||
There was a problem hiding this comment.
is it necessary to hold the lock while calling mkgrent?
There was a problem hiding this comment.
Oh, hm, I thought this was a grp function. You're right--I don't think it's safe to hold the lock while calling it, because it executes finalizers.
There was a problem hiding this comment.
is it necessary to hold the lock while calling mkgrent?
I think we should hold the lock while processing a pointer to a struct group
The returned pointer, and pointers within the structure, might be invalidated or the structure or the storage areas might be overwritten by a subsequent call to
getgrent(),getgrgid(), orgetgrnam(). (link)
There was a problem hiding this comment.
Oh, hm, I thought this was a grp function. You're right--I don't think it's safe to hold the lock while calling it, because it executes finalizers.
Lines 294 to 298 in da79ac9
grp_getgrall_impl() holds the lock around mkgrent(), but I'll check if it's something we need to fix.
There was a problem hiding this comment.
might be overwritten by a subsequent call to getgrent(), getgrgid(), or getgrnam()
When I re-read the document, I think we need one lock to protect all those function calls. I'll update the PR.
There was a problem hiding this comment.
Ok, so we're intentionally playing with fire a little bit. I think the lock is fine, then.
Make
grpmodule methodsgetgrgid()andgetgrnam()thread-safe when the GIL is disabled andgetgrgid_r()/getgrnam_r()C APIs are not available.run_concurrently()totest.support.threading_helperto resuse across multiple tests.cc: @mpage @colesbury @Yhg1s