bpo-39776: Lock ++interp->tstate_next_unique_id. (GH-18746) #18746

skrah · 2020-03-02T17:03:14Z

Threads created by PyGILState_Ensure() could have a duplicate tstate->id.

https://bugs.python.org/issue39776

- Threads created by PyGILState_Ensure() could have a duplicate tstate->id.

vstinner · 2020-03-02T17:12:16Z

Oh. That's an interesting bug :-) Would you mind to add a NEWS entry?

The fix should be backported to 3.7 and 3.8 branches. It's a severe bug, likely very hard to reproduce :-(

codecov · 2020-03-02T17:35:11Z

Codecov Report

Merging #18746 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #18746   +/-   ##
=======================================
  Coverage   82.13%   82.14%           
=======================================
  Files        1956     1956           
  Lines      590161   590206   +45     
  Branches    44493    44503   +10     
=======================================
+ Hits       484758   484798   +40     
  Misses      95751    95751           
- Partials     9652     9657    +5

Impacted Files	Coverage Δ
Lib/idlelib/runscript.py	`19.23% <0.00%> (-1.54%)`	⬇️
Lib/test/test_asyncio/functional.py	`70.27% <0.00%> (-1.09%)`	⬇️
Python/context.c	`80.16% <0.00%> (-1.01%)`	⬇️
Modules/selectmodule.c	`83.53% <0.00%> (-1.00%)`	⬇️
Lib/test/test_asyncio/test_sock_lowlevel.py	`89.08% <0.00%> (-0.88%)`	⬇️
Lib/filecmp.py	`64.81% <0.00%> (-0.62%)`	⬇️
Lib/test/test_asyncio/utils.py	`87.39% <0.00%> (-0.57%)`	⬇️
Lib/asyncio/unix_events.py	`81.48% <0.00%> (-0.24%)`	⬇️
Lib/ast.py	`74.08% <0.00%> (-0.15%)`	⬇️
Lib/test/test_io.py	`94.86% <0.00%> (-0.04%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66b7973...a9b9e8a. Read the comment docs.

pitrou · 2020-03-02T18:29:22Z

Python/pystate.c

    if (init) {
        _PyThreadState_Init(tstate);
    }

    HEAD_LOCK(runtime);
+    tstate->id = ++interp->tstate_next_unique_id;


Ha, is the reason for the problem that the increment was done unlocked?

skrah · 2020-03-02T19:31:25Z

@vstinner Yes, it deserves a blurb. :)

@pitrou Yes, I printed tstate->id in the reproducer code and it had tons of duplicates. So

cpython/Python/context.c

Line 201 in 89aa469

var->var_cached_tsid == ts->id &&

sometimes returned cached values that had already been deallocated by a previous thread
with a duplicate id.

With this fix there are no longer any duplicates and the test does not crash. If you review,
could you glance at https://bugs.python.org/file48930/threaded_crash.zip and verify that
it is actually correct embedding code (I think it is). :)

ASAN in release mode still complains about another invalid read in ceval.c, but I
think that is unrelated (it is also present in master).

pitrou · 2020-03-02T19:33:05Z

Yes, threaded_crash.zip seems to be ok.

skrah · 2020-03-02T19:33:25Z

Python/pystate.c

    if (init) {
        _PyThreadState_Init(tstate);
    }

    HEAD_LOCK(runtime);
+    tstate->id = ++interp->tstate_next_unique_id;


The location is unfortunate, but _PyThreadState_Init(tstate) does not use this field.

pitrou

+1 from me.

bedevere-bot · 2020-03-02T20:22:40Z

@skrah: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2020-03-02T20:22:40Z

Thanks @skrah for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2020-03-02T20:22:42Z

Sorry @skrah, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker b3b9ade4a3d3fe00d933bcd8fc5c5c755d1024f9 3.8

miss-islington · 2020-03-02T20:22:46Z

Sorry, @skrah, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b3b9ade4a3d3fe00d933bcd8fc5c5c755d1024f9 3.7

skrah · 2020-03-02T20:25:19Z

Thanks! Backports will follow ...

bedevere-bot · 2020-03-02T23:25:11Z

GH-18752 is a backport of this pull request to the 3.8 branch.

bedevere-bot · 2020-03-02T23:27:07Z

GH-18753 is a backport of this pull request to the 3.7 branch.

…#18752) - Threads created by PyGILState_Ensure() could have a duplicate tstate->id. (cherry picked from commit b3b9ade)

- Threads created by PyGILState_Ensure() could have a duplicate tstate->id. (cherry picked from commit b3b9ade)

bpo-39776: Lock ++interp->tstate_next_unique_id.

cd42fa4

- Threads created by PyGILState_Ensure() could have a duplicate tstate->id.

the-knights-who-say-ni added the CLA signed label Mar 2, 2020

bedevere-bot added the awaiting core review label Mar 2, 2020

skrah changed the title ~~bpo-39776: Lock ++interp->tstate_next_unique_id.~~ bpo-39776: Lock ++interp->tstate_next_unique_id. (GH-18746) Mar 2, 2020

pitrou reviewed Mar 2, 2020

View reviewed changes

Add blurb.

a9b9e8a

skrah commented Mar 2, 2020

View reviewed changes

pitrou approved these changes Mar 2, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Mar 2, 2020

pitrou added needs backport to 3.7 labels Mar 2, 2020

skrah merged commit b3b9ade into python:master Mar 2, 2020

bedevere-bot removed the awaiting merge label Mar 2, 2020

miss-islington assigned skrah Mar 2, 2020

bedevere-bot removed the needs backport to 3.8 label Mar 2, 2020

bedevere-bot removed the needs backport to 3.7 label Mar 2, 2020

skrah added a commit that referenced this pull request Mar 3, 2020

bpo-39776: Lock ++interp->tstate_next_unique_id. (GH-18746) (#18746) (…

5a92f42

…#18752) - Threads created by PyGILState_Ensure() could have a duplicate tstate->id. (cherry picked from commit b3b9ade)

skrah added a commit that referenced this pull request Mar 3, 2020

bpo-39776: Lock ++interp->tstate_next_unique_id (GH-18746)

852aee6

- Threads created by PyGILState_Ensure() could have a duplicate tstate->id. (cherry picked from commit b3b9ade)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39776: Lock ++interp->tstate_next_unique_id. (GH-18746) #18746

bpo-39776: Lock ++interp->tstate_next_unique_id. (GH-18746) #18746

skrah commented Mar 2, 2020 •

edited by bedevere-bot

Loading

vstinner commented Mar 2, 2020

codecov bot commented Mar 2, 2020 •

edited

Loading

pitrou Mar 2, 2020

skrah commented Mar 2, 2020

pitrou commented Mar 2, 2020

skrah Mar 2, 2020 •

edited

Loading

pitrou left a comment

bedevere-bot commented Mar 2, 2020

miss-islington commented Mar 2, 2020

miss-islington commented Mar 2, 2020

miss-islington commented Mar 2, 2020

skrah commented Mar 2, 2020

bedevere-bot commented Mar 2, 2020

bedevere-bot commented Mar 2, 2020

bpo-39776: Lock ++interp->tstate_next_unique_id. (GH-18746) #18746

bpo-39776: Lock ++interp->tstate_next_unique_id. (GH-18746) #18746

Conversation

skrah commented Mar 2, 2020 • edited by bedevere-bot Loading

vstinner commented Mar 2, 2020

codecov bot commented Mar 2, 2020 • edited Loading

Codecov Report

pitrou Mar 2, 2020

Choose a reason for hiding this comment

skrah commented Mar 2, 2020

pitrou commented Mar 2, 2020

skrah Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

bedevere-bot commented Mar 2, 2020

miss-islington commented Mar 2, 2020

miss-islington commented Mar 2, 2020

miss-islington commented Mar 2, 2020

skrah commented Mar 2, 2020

bedevere-bot commented Mar 2, 2020

bedevere-bot commented Mar 2, 2020

skrah commented Mar 2, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Mar 2, 2020 •

edited

Loading

skrah Mar 2, 2020 •

edited

Loading