Repository navigation
grp is not thread safe #126316
Description
Crash report
What happened?
It's possible to crash grp in a no-gil build by repeatedly calling functions in threads, it only happens with PYTHON_GIL=0.
from threading import Thread
import grp
for x in range(5000):
alive = [
Thread(target=grp.getgrgid, args=(1,)),
Thread(target=grp.getgrall),
Thread(target=grp.getgrnam, args=('root',)),
]
for obj in alive:
print('START', obj)
obj.start()Backtrace is:
(gdb) bt
#0 __strlen_avx2 ()
at ../sysdeps/x86_64/multiarch/strlen-avx2.S:74
#1 0x00005555557ad3cb in PyUnicode_DecodeFSDefault (
s=s@entry=0x3838323432353a78 <error: Cannot access memory at address 0x3838323432353a78>) at Objects/unicodeobject.c:4058
#2 0x00007ffff7c3f952 in mkgrent (
module=module@entry=0x20000795b80, p=<optimized out>)
at ./Modules/grpmodule.c:83
#3 0x00007ffff7c3fc41 in grp_getgrall_impl (
module=0x20000795b80) at ./Modules/grpmodule.c:291
#4 0x00007ffff7c3fcad in grp_getgrall (
module=<optimized out>, _unused_ignored=<optimized out>)
at ./Modules/clinic/grpmodule.c.h:83
#5 0x00005555556fca83 in cfunction_vectorcall_NOARGS (
func=0x2000079f820, args=<optimized out>,
nargsf=<optimized out>, kwnames=<optimized out>)
at Objects/methodobject.c:495
#6 0x000055555567b4bc in _PyVectorcall_Call (
tstate=tstate@entry=0x555555d2ac70,
func=0x5555556fc8b5 <cfunction_vectorcall_NOARGS>,
callable=callable@entry=0x2000079f820,
tuple=tuple@entry=0x555555c5d1d8 <_PyRuntime+128984>,
kwargs=kwargs@entry=0x2001a0400d0) at Objects/call.c:273
#7 0x000055555567b894 in _PyObject_Call (
tstate=0x555555d2ac70,
callable=callable@entry=0x2000079f820,
args=args@entry=0x555555c5d1d8 <_PyRuntime+128984>,
kwargs=kwargs@entry=0x2001a0400d0) at Objects/call.c:348
#8 0x000055555567b8f1 in PyObject_Call (
callable=callable@entry=0x2000079f820,
args=args@entry=0x555555c5d1d8 <_PyRuntime+128984>,
kwargs=kwargs@entry=0x2001a0400d0) at Objects/call.c:373Found using fusil by @vstinner.
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Output from running 'python -VV' on the command line:
Python 3.14.0a1+ experimental free-threading build (heads/main:d467d9246c, Nov 1 2024, 09:05:56) [GCC 11.4.0]
Linked PRs
- gh-126316: Use critical sections in grp module #126488
- gh-126316: Use mutexes in the grp module #126504
- gh-126316: Use getgrent_r() in grp.getgrall() #126506
- gh-126316: Make grp.getgrall() thread-safe: add a mutex #127055
- [3.13] gh-126316: Make grp.getgrall() thread-safe: add a mutex (#127055) #127104
Activity
colesbury commented on Nov 2, 2024
Thanks for the bug reports. What is fusil?
devdanzin commented on Nov 2, 2024
fusil is a fuzzer written in Python by Victor Stinner around 2007 I think. Somehow it has been memory holed, but some references can still be found in the WayBack Machine:
RTD
Home
List of Python bugs found (way back when)
Articles by Victor in French
I used it in the old days to find a couple of issues and was curious whether it could still find some. The source is still available even if not so easy to find. I got it, fixed some issues, added a bit of (very rough) threading, and let it search for crashes.
Not sure what to do with the code, first I'd like to know why it disappeared. But I'd be happy to share privately.
colesbury commented on Nov 2, 2024
From a quick glance, it looks like getgrall uses setgrent(), getgrent(), endgrent(), which are not thread-safe on Linux.
The macOS implementations appear to be thread-safe
Linux with glibc 2.19+ has fgetgrent_r, which is thread-safe.
kumaraditya303 commented on Nov 6, 2024
As a quick fix, I think the module should be marked with Py_MOD_GIL_USED
pythongh-126316: Use critical sections in grp module
pythongh-126316: Use critical sections in grp module
pythongh-126316: Use critical sections in grp module
vstinner commented on Nov 6, 2024
I wrote a fix #126488 using @critical_section.
kumaraditya303 commented on Nov 6, 2024
I don't think that would fix the issue, there can be two instances of module which can race with each other. The correct fix would be to have a global lock which would serialize calls across modules.
ZeroIntensity commented on Nov 6, 2024
Yeah, this will still race if there are subinterpreters using grp at the same time. Kumar's suggestion is probably the way to go.
14 remaining items
vstinner commented on Nov 6, 2024
Ok, so apparently, only grp.getgrall() is not thread-safe, and getgrent_r() can be used instead of getgrent() to make the function reentrant. I didn't want to use fgetgrent_r() to not have to open a file (which file?).
In short, I wrote PR gh-126506 to fix the issue using getgrent_r().
colesbury commented on Nov 6, 2024
Ugh, getgrent_r is not thread-safe on Linux with glibc. Yuck. And fgetgrent_r isn't a good substitute if groups use LDAP or anything that's not /etc/groups.
I guess locks it is... at least for Linux.
vstinner commented on Nov 20, 2024
I proposed @critical_section (PR gh-126488), but @ZeroIntensity wrote that it's not correct since it doesn't solve the problem for subinterpreters which have their GIL.
I proposed adding PyMutex (PR gh-126504), but @colesbury is afraid of deadlock because "mkgrent() function calls a into the Python API". Maybe I can release the mutex while calling mkgrent()?
I also wrote PR gh-126506 to use getgrent_r(), but @colesbury noticed that getgrent_r() is not thread-safe on Linux, so it doesn't solve this issue.
I suppose that the only solution is to use a lock and release it around mkgrent().
pythongh-126316: Make grp.getgrall() thread-safe: add a mutex
pythongh-126316: Make grp.getgrall() thread-safe: add a mutex
gh-126316: Make grp.getgrall() thread-safe: add a mutex (#127055)
pythongh-126316: Make grp.getgrall() thread-safe: add a mutex (python…
vstinner commented on Nov 21, 2024
Fixed by change 3c2bd66. Backport to 3.13 will follow.
vstinner commented on Nov 21, 2024
Thank you @devdanzin for your bug report.
[3.13] gh-126316: Make grp.getgrall() thread-safe: add a mutex (#127055…
pythongh-126316: Make grp.getgrall() thread-safe: add a mutex (python…