Skip to content

PyOS_Readline crashes in a multi-threaded race #123321

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.

Sign up for GitHub

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

Jump to bottom
Closed
bharel opened this issue Aug 25, 2024 · 8 comments
Closed

PyOS_Readline crashes in a multi-threaded race #123321

bharel opened this issue Aug 25, 2024 · 8 comments
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@bharel
Copy link
Contributor

bharel commented Aug 25, 2024 •
edited by bedevere-app bot
Loading

Crash report

What happened?

The second breakpoint segfaults on Windows (==access violation).

import asyncio
async def main():
    def inner():
        breakpoint()
        pass
    asyncio.create_task(asyncio.to_thread(inner))
    asyncio.create_task(asyncio.to_thread(inner))

asyncio.ensure_future(main())
asyncio.get_event_loop().run_forever()

Core of the problem is in the myreadline.c module.
Implementing a fix...

CPython versions tested on:

3.12, CPython main branch

Operating systems tested on:

Windows

Output from running 'python -VV' on the command line:

No response

Linked PRs
@bharel bharel added the type-crash A hard crash of the interpreter, possibly with a core dump label Aug 25, 2024
@bedevere-app bedevere-app bot mentioned this issue Aug 25, 2024
Merged
@gaogaotiantian
Copy link
Member

Do you actually need asyncio and pdb to repro this? If this is a readline issue with multi-thread, it would be nicer if the regression test is closer to the actual problem. Can you repro this with pure readline and threading? Both pdb and asyncio is complicated enough that dropping them off on a regression test might be helpful in the future.

@bharel
Copy link
Contributor Author

bharel commented Aug 26, 2024 •
edited
Loading

@gaogaotiantian I completely agree with your insight. The problem is that I've attempted achieving it with only threading and pdb, and for some reason it doesn't reproduce.

Theoretically according to the faulting code it should (and should happen much more often). I'll give it another go.

Haven't thought of testing purely with readline. Didn't notice the separate tests.

@bharel
Copy link
Contributor Author

bharel commented Aug 26, 2024

By the way, out of interest, do you know why the free threading build fails on altering the execution environment? What does it mean?

@ZeroIntensity
Copy link
Member

I'm assuming that this also occurs with asyncio.run? Calling get_event_loop like that is deprecated.

@bharel
Copy link
Contributor Author

bharel commented Aug 26, 2024

Yeah, I managed to reproduce using input() and threading.

@gaogaotiantian
Copy link
Member

That's great. I think you can update the test case and put in the file together with other readline tests. I'm not the expert on free-threaded build so I'm afraid I can't help in that area.

@bharel
Copy link
Contributor Author

bharel commented Aug 28, 2024

For reference, here's the simplest piece of code that triggers the issue:

import threading

def main():
    def func():
        input()
    thread1 = threading.Thread(target=func)
    thread2 = threading.Thread(target=func)
    thread1.start()
    thread2.start()
    thread1.join()
    thread2.join()
    print("never")

main()

@picnixz picnixz changed the title PDB segfault on thread close PyOS_Readline crashes in a multi-threaded race Aug 28, 2024
@picnixz picnixz added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes labels Aug 28, 2024
bharel added a commit to bharel/cpython that referenced this issue Aug 30, 2024
@bharel
Merge branch 'main' into pythongh-123321
934e21f
@bharel
Copy link
Contributor Author

bharel commented Aug 30, 2024 •
edited
Loading

@gaogaotiantian @ZeroIntensity fix is ready.

I appreciate your comments :-)

bharel added a commit to bharel/cpython that referenced this issue Sep 1, 2024
@bharel
Merge branch 'main' into pythongh-123321
c083eba
bharel added a commit to bharel/cpython that referenced this issue Sep 3, 2024
@bharel
Merge branch 'main' into pythongh-123321
cf1b274
vstinner pushed a commit that referenced this issue Sep 4, 2024
@bharel
gh-123321: Fix Parser/myreadline.c to prevent a segfault during a mul…
a4562fe 
…ti-threaded race (#123323)
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 4, 2024
@bharel @miss-islington
pythongh-123321: Fix Parser/myreadline.c to prevent a segfault during…
683f8db 
… a multi-threaded race (pythonGH-123323)

(cherry picked from commit a4562fe)

Co-authored-by: Bar Harel <bharel@barharel.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 4, 2024
@bharel @miss-islington
pythongh-123321: Fix Parser/myreadline.c to prevent a segfault during…
bf05ff9 
… a multi-threaded race (pythonGH-123323)

(cherry picked from commit a4562fe)

Co-authored-by: Bar Harel <bharel@barharel.com>
This was referenced Sep 4, 2024
Merged
Merged
pablogsal pushed a commit that referenced this issue Sep 4, 2024
@miss-islington @bharel
[3.13] gh-123321: Fix Parser/myreadline.c to prevent a segfault durin…
9193d29 
…g a multi-threaded race (GH-123323) (#123676)

gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race (GH-123323)
(cherry picked from commit a4562fe)

Co-authored-by: Bar Harel <bharel@barharel.com>
colesbury added a commit to colesbury/cpython that referenced this issue Sep 4, 2024
@colesbury
pythongh-123321: Make Parser/myreadline.c locking safe in free-thread…
3976267 
…ed build

Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed
atomics to avoid the data race on reading _PyOS_ReadlineTState when
checking for re-entrant calls.
@bedevere-app bedevere-app bot mentioned this issue Sep 4, 2024
Merged
vstinner pushed a commit that referenced this issue Sep 5, 2024
@miss-islington @bharel @colesbury
[3.12] gh-123321: Fix Parser/myreadline.c to prevent a segfault durin…
562ff73 
…g a multi-threaded race (GH-123323) (#123677)

* gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race (GH-123323)
(cherry picked from commit a4562fe)

Co-authored-by: Bar Harel <bharel@barharel.com>

* Remove @requires_gil_enabled for 3.12

---------

Co-authored-by: Bar Harel <bharel@barharel.com>
Co-authored-by: Sam Gross <colesbury@gmail.com>
colesbury added a commit that referenced this issue Sep 6, 2024
@colesbury
gh-123321: Make Parser/myreadline.c locking safe in free-threaded bui…
0c080d7 
…ld (#123690)

Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed
atomics to avoid the data race on reading `_PyOS_ReadlineTState` when
checking for re-entrant calls.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 6, 2024
@colesbury @miss-islington
pythongh-123321: Make Parser/myreadline.c locking safe in free-thread…
2c48879 
…ed build (pythonGH-123690)

Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed
atomics to avoid the data race on reading `_PyOS_ReadlineTState` when
checking for re-entrant calls.
(cherry picked from commit 0c080d7)

Co-authored-by: Sam Gross <colesbury@gmail.com>
@bedevere-app bedevere-app bot mentioned this issue Sep 6, 2024
Merged
Yhg1s pushed a commit that referenced this issue Sep 6, 2024
@miss-islington @colesbury
[3.13] gh-123321: Make Parser/myreadline.c locking safe in free-threa…
9052888 
…ded build (GH-123690) (#123798)

gh-123321: Make Parser/myreadline.c locking safe in free-threaded build (GH-123690)

Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed
atomics to avoid the data race on reading `_PyOS_ReadlineTState` when
checking for re-entrant calls.
(cherry picked from commit 0c080d7)

Co-authored-by: Sam Gross <colesbury@gmail.com>
@rohitsanj rohitsanj mentioned this issue Apr 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bharel @picnixz @gaogaotiantian @ZeroIntensity