Skip to content

grp is not thread safe #126316

Closed
Closed
grp is not thread safe#126316
Labels
3.13only security fixes3.14bugs and security fixesextension-modulesC modules in the Modules dirtopic-free-threadingtopic-subinterpreterstype-crashA hard crash of the interpreter, possibly with a core dump

Description

@devdanzin
devdanzin
opened on Nov 1, 2024

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:373

Found 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

Activity

  1. devdanzin
    added
    type-crashA hard crash of the interpreter, possibly with a core dump
    on Nov 1, 2024
  2. picnixz
    added
    extension-modulesC modules in the Modules dir
    topic-free-threading
    on Nov 2, 2024

  3. colesbury commented on Nov 2, 2024

    @colesbury
    colesbury
    on Nov 2, 2024
    Contributor

    Thanks for the bug reports. What is fusil?

  4. devdanzin commented on Nov 2, 2024

    @devdanzin
    devdanzin
    on Nov 2, 2024
    MemberAuthor

    What is fusil?

    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.

  5. colesbury commented on Nov 2, 2024

    @colesbury
    colesbury
    on Nov 2, 2024
    Contributor

    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.

  6. kumaraditya303 commented on Nov 6, 2024

    @kumaraditya303
    kumaraditya303
    on Nov 6, 2024
    Contributor

    As a quick fix, I think the module should be marked with Py_MOD_GIL_USED

  7. vstinner
    added 3 commits that reference this issue on Nov 6, 2024

    pythongh-126316: Use critical sections in grp module

    c898b70

    pythongh-126316: Use critical sections in grp module

    ede8bc4

    pythongh-126316: Use critical sections in grp module

    d56383c

  8. vstinner commented on Nov 6, 2024

    @vstinner
    vstinner
    on Nov 6, 2024
    Member

    As a quick fix, I think the module should be marked with Py_MOD_GIL_USED

    I wrote a fix #126488 using @critical_section.

  9. kumaraditya303 commented on Nov 6, 2024

    @kumaraditya303
    kumaraditya303
    on Nov 6, 2024
    Contributor

    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.

  10. ZeroIntensity commented on Nov 6, 2024

    @ZeroIntensity
    ZeroIntensity
    on Nov 6, 2024
    Member

    Yeah, this will still race if there are subinterpreters using grp at the same time. Kumar's suggestion is probably the way to go.

  11. ZeroIntensity
    added
    topic-subinterpreters
    on Nov 6, 2024
  12. github-project-automation
    added this to Subinterpreterson Nov 6, 2024

  13. 14 remaining items

  14. vstinner commented on Nov 6, 2024

    @vstinner
    vstinner
    on Nov 6, 2024
    Member

    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().

  15. colesbury commented on Nov 6, 2024

    @colesbury
    colesbury
    on Nov 6, 2024
    Contributor

    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.

  16. picnixz
    added
    3.13only security fixes
    3.14bugs and security fixes
    on Nov 16, 2024

  17. vstinner commented on Nov 20, 2024

    @vstinner
    vstinner
    on Nov 20, 2024
    Member

    I guess locks it is... at least for Linux.

    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().

  18. vstinner
    added 4 commits that reference this issue on Nov 20, 2024

    pythongh-126316: Make grp.getgrall() thread-safe: add a mutex

    d399293

    pythongh-126316: Make grp.getgrall() thread-safe: add a mutex

    40744e4

    gh-126316: Make grp.getgrall() thread-safe: add a mutex (#127055)

    3c2bd66

    pythongh-126316: Make grp.getgrall() thread-safe: add a mutex (python…

    c36d77d

  19. vstinner commented on Nov 21, 2024

    @vstinner
    vstinner
    on Nov 21, 2024
    Member

    Fixed by change 3c2bd66. Backport to 3.13 will follow.

  20. vstinner
    closed this as completedon Nov 21, 2024
  21. github-project-automation
    moved this from Todo to Done in Subinterpreterson Nov 21, 2024

  22. vstinner commented on Nov 21, 2024

    @vstinner
    vstinner
    on Nov 21, 2024
    Member

    Thank you @devdanzin for your bug report.

  23. vstinner
    added a commit that references this issue on Nov 26, 2024

    [3.13] gh-126316: Make grp.getgrall() thread-safe: add a mutex (#127055…

    9187ac3
  24. ebonnal
    added a commit that references this issue on Jan 12, 2025

    pythongh-126316: Make grp.getgrall() thread-safe: add a mutex (python…

    2629000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.13only security fixes3.14bugs and security fixesextension-modulesC modules in the Modules dirtopic-free-threadingtopic-subinterpreterstype-crashA hard crash of the interpreter, possibly with a core dump

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions