gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race #123323

bharel · 2024-08-25T22:09:59Z

The variable is now set and reset only within the lock.

Previously, it was a global variable that was reset outside of the lock and caused an access violation at line 265:

if (!config->legacy_windows_stdio && sys_stdin == stdin)

Happened with higher chance during thread closure. tstate at line 260 turned null, which caused the config to be null.

I'd appreciate help with the tests, I chose to test it on PDB but it's a difficult catch.
I also do not have a Windows machine readily available which is required to trigger.

Issue: PyOS_Readline crashes in a multi-threaded race #123321

bharel · 2024-08-25T22:12:35Z

By the way, I believe there's an issue with the pdb tests, I'm not sure if the doctests actually run or not.

colesbury · 2024-08-26T21:22:02Z

Lib/test/test_pdb.py

@@ -2731,6 +2731,46 @@ def test_pdb_issue_gh_103225():
    (Pdb) continue
    """

+if not SKIP_ASYNCIO_TESTS:
+    def test_pdb_issue_gh_123321():


I think this test is failing in the free-threaded build, probably because the initialization of the _PyOS_ReadlineLock lock isn't thread-safe without the GIL (and a few other related issues).

That shouldn't be too hard to fix, but we should do it after this PR has landed because the bug you are fixing exists in 3.12, and the free-threading fixes should only go back to 3.13.

In the meantime, you can add a @support.requires_gil_enabled() decorator to skip this test in the free-threaded build.

Oh thanks, I'll do that.
I'm changing the tests now and moving them to the readline or builtins module.

Was fixed when I took the lock inside the GIL.

I was worried about a potential deadlock, but I don't see any other uses for the lock except this function so it's fine.

Actually it does deadlock:

First thread:

GIL.acq() -> lock.acq() -> GIL.rls() -> |GIL.acq()| -> lock.rls()

Second thread:
GIL.acq() -> |lock.acq()| -> GIL.rls() -> GIL.acq() -> lock.rls()

Taking the lock before releasing the GIL will require releasing the lock without the GIL, which is very dangerous if the internal function acquires the GIL in any way. i.e. this will work, but dangerous if GIL is taken again after the release:

GIL.acq() -> lock.acq() -> GIL.rls() -> |dangerous if GIL taken here| -> lock.rls() -> GIL.acq()

I will have to take the lock outside the GIL and we need to ensure that taking the lock is threadsafe outside the GIL.

vstinner · 2024-08-30T15:50:12Z

Parser/myreadline.c

    PyThread_acquire_lock(_PyOS_ReadlineLock, 1);
+    Py_BEGIN_ALLOW_THREADS
+    _PyOS_ReadlineTState = tstate;


Maybe set _PyOS_ReadlineTState while you are still holding the GIL, before Py_BEGIN_ALLOW_THREADS. It should not make a big difference, since other threads should be blocked at PyThread_acquire_lock(_PyOS_ReadlineLock, 1) :-)

Alrighty :-)

So we have to release the lock outside the GIL and preferably take the lock too, otherwise it deadlocks.

It fails on the free-threaded as according to @colesbury the taking of the lock isn't safe outside the GIL.

I'm a little confused as theoretically if it isn't safe it should fail on the normal build too but I'm not entirely sure how that works.

Parser/myreadline.c

…ks. Shifted tests to the readline module.

Lib/test/test_readline.py

vstinner

LGTM.

If I understood @colesbury correctly, the @requires_gil_enabled() decorator avoids a test failure on Free Threaded build for now, and so the fix is good enough for "Python with GIL (enabled)".

Lib/test/test_readline.py

bharel · 2024-08-30T22:06:59Z

LGTM.

If I understood @colesbury correctly, the @requires_gil_enabled() decorator avoids a test failure on Free Threaded build for now, and so the fix is good enough for "Python with GIL (enabled)".

So here's the thing - if it requires the GIL in order to take the lock then it shouldn't work in the normal build after ALLOW_THREADS. If it doesn't require the GIL and it is threadsafe (atomic / other lock) then it should work on both builds.

I fear that it isn't safe outside the GIL and then we create a new issue that we aren't aware of. If it is then the Free Threaded should work too.

Either way, how would one solve it in the free threaded build? Shall I open a new issue for that or will there be a large undertaking to take care of everything containing that decorator at a later time?

pablogsal · 2024-09-03T12:30:09Z

Parser/myreadline.c


+    // gh-123321: Must set the variable and then release the lock before
+    // taking the GIL. Otherwise a deadlock or segfault may occur.


How can a deadlock occur due to this variable not being NULL?

Release lock then take GIL.
If you take GIL and then release lock there's a deadlock.

If you set the variable before releasing the lock you segfault.

Ah the link you posted is exactly what I wanted to know. Thanks!

colesbury · 2024-09-03T14:57:01Z

If I understood colesbury correctly, the @requires_gil_enabled() decorator avoids a test failure on Free Threaded build for now, and so the fix is good enough for "Python with GIL (enabled)".

Yeah, the fix for the free-threaded build will require some code that's not available in 3.12. So my thinking is that we fix it first of for the default build like this and backport it all the way to 3.12, and then I can put up a PR that fixes it for the free-threaded build (and removes the skip annotation). That one won't get backported to 3.12.

vstinner · 2024-09-04T11:45:21Z

@colesbury @pablogsal: Are you ok with this change?

miss-islington-app · 2024-09-04T15:22:39Z

Thanks @bharel for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2024-09-04T15:22:39Z

Thanks @bharel for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

… a multi-threaded race (pythonGH-123323) (cherry picked from commit a4562fe) Co-authored-by: Bar Harel <[email protected]>

bedevere-app · 2024-09-04T15:22:48Z

GH-123676 is a backport of this pull request to the 3.13 branch.

bedevere-app · 2024-09-04T15:22:52Z

GH-123677 is a backport of this pull request to the 3.12 branch.

bharel · 2024-09-04T15:39:51Z

Sweet. I'd appreciate a mention and a link to this issue in the free threaded build.

Thanks everyone for the quick review and suggestions, amazing work 🥂

And to @amit-w for helping me debug it ❤️

…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 <[email protected]>

…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 <[email protected]> * Remove @requires_gil_enabled for 3.12 --------- Co-authored-by: Bar Harel <[email protected]> Co-authored-by: Sam Gross <[email protected]>

patch

360637f

bedevere-app bot mentioned this pull request Aug 25, 2024

PyOS_Readline crashes in a multi-threaded race #123321

Open

whitespace

8f16d5a

bharel added 7 commits August 26, 2024 01:15

hmmm pdb doctests don't run

1f5c558

for some reason the doctests don't run locally

8a8c355

doctest fix

ed58ecb

mmm

db92111

this will probably work.

b991cd6

typo

26ad6bc

ugh

03243d0

colesbury reviewed Aug 26, 2024

View reviewed changes

small check for free-threaded build

6b0b820

vstinner reviewed Aug 30, 2024

View reviewed changes

bharel added 4 commits August 30, 2024 23:12

Reverting to previous solution as the free-threaded build fix deadloc…

40efaac

…ks. Shifted tests to the readline module.

lint

fc40da3

remove pdb tests

bebc580

Merge branch 'main' into pythongh-123321

934e21f

bharel marked this pull request as ready for review August 30, 2024 20:31

bharel requested review from pablogsal and lysnikolaou as code owners August 30, 2024 20:31

bedevere-app bot added the awaiting review label Aug 30, 2024

Add requires gil enabled tag

0379488

vstinner reviewed Aug 30, 2024

View reviewed changes

Lib/test/test_readline.py Outdated Show resolved Hide resolved

Lib/test/test_readline.py Outdated Show resolved Hide resolved

CR fixes.

fd2929c

vstinner approved these changes Aug 30, 2024

View reviewed changes

Lib/test/test_readline.py Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting review labels Aug 30, 2024

.

93dd023

bharel added 2 commits September 1, 2024 03:44

Merge branch 'main' into pythongh-123321

c083eba

Merge branch 'main' into pythongh-123321

cf1b274

pablogsal reviewed Sep 3, 2024

View reviewed changes

colesbury approved these changes Sep 4, 2024

View reviewed changes

vstinner merged commit a4562fe into python:main Sep 4, 2024
37 checks passed

bedevere-app bot removed the awaiting merge label Sep 4, 2024

vstinner added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Sep 4, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 4, 2024

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Sep 4, 2024

bharel deleted the gh-123321 branch September 5, 2024 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race #123323

gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race #123323

bharel commented Aug 25, 2024 •

edited by bedevere-app bot

Loading

bharel commented Aug 25, 2024

colesbury Aug 26, 2024

bharel Aug 26, 2024

bharel Aug 30, 2024

bharel Aug 30, 2024 •

edited

Loading

vstinner Aug 30, 2024

bharel Aug 30, 2024

bharel Aug 30, 2024

vstinner left a comment

bharel commented Aug 30, 2024 •

edited

Loading

pablogsal Sep 3, 2024

bharel Sep 3, 2024 •

edited

Loading

pablogsal Sep 3, 2024

colesbury commented Sep 3, 2024

vstinner commented Sep 4, 2024

miss-islington-app bot commented Sep 4, 2024

miss-islington-app bot commented Sep 4, 2024

bedevere-app bot commented Sep 4, 2024

bedevere-app bot commented Sep 4, 2024

bharel commented Sep 4, 2024 •

edited

Loading


		// gh-123321: Must set the variable and then release the lock before
		// taking the GIL. Otherwise a deadlock or segfault may occur.

gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race #123323

gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race #123323

Conversation

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

bharel commented Aug 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharel Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

bharel commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

bharel Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesbury commented Sep 3, 2024

vstinner commented Sep 4, 2024

miss-islington-app bot commented Sep 4, 2024

miss-islington-app bot commented Sep 4, 2024

bedevere-app bot commented Sep 4, 2024

bedevere-app bot commented Sep 4, 2024

bharel commented Sep 4, 2024 • edited Loading

bharel commented Aug 25, 2024 •

edited by bedevere-app bot

Loading

bharel Aug 30, 2024 •

edited

Loading

bharel commented Aug 30, 2024 •

edited

Loading

bharel Sep 3, 2024 •

edited

Loading

bharel commented Sep 4, 2024 •

edited

Loading