gh-128657: fix _hashopenssl ref/data race by tom-pytel · Pull Request #128886 · python/cpython

tom-pytel · 2025-01-15T15:43:08Z

Fix a possible data and PY_EVP_MD refcount race in _hashopenssl.c in py_digest_by_name() under free-threading.

Issue: Race in py_digest_by_name, _hashopenssl.c under free-threading #128657

bedevere-app · 2025-01-15T15:43:16Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Misc/NEWS.d/next/Library/2025-01-15-15-45-21.gh-issue-128657.P5LNQA.rst

Modules/_hashopenssl.c

ZeroIntensity · 2025-01-16T14:24:03Z

The implementation LGTM. You can reuse the reproducer in the issue as a test. See the devguide for writing tests.

tom-pytel · 2025-01-16T14:58:38Z

The implementation LGTM. You can reuse the reproducer in the issue as a test. See the devguide for writing tests.

Are TSAN tests possible? Because that is the only way the issue shows up, other than as an extra refcount at a specific byte offset of the opaque evp_md_st sturct IF two threads do this simultaneously (very non-deterministic).

ZeroIntensity · 2025-01-16T15:00:10Z

Yeah, tests are run with TSan as part of CI. For example, on this PR: https://github.com/python/cpython/actions/runs/12809851053/job/35715616858?pr=128886

tom-pytel · 2025-01-16T15:59:14Z

Yeah, tests are run with TSan as part of CI. For example, on this PR: https://github.com/python/cpython/actions/runs/12809851053/job/35715616858?pr=128886

Still not sure how you want me to implement this test. python -m test --tsan appears to exist with the intent to test the thread sanitizer itself and I can't find any actual test that interacts with tsan to get positive or negative results? Can you point me to one, because I have a test written but no way to know if it succeeds or fails according to tsan.

ZeroIntensity · 2025-01-16T16:20:55Z

You don't need to do anything special--just write the test. Python will be built with TSan integration, and races on that test will show up in CI.

tom-pytel · 2025-01-16T16:52:12Z

You don't need to do anything special--just write the test. Python will be built with TSan integration, and races on that test will show up in CI.

Done. Had to add test_hashlib to list of tsan tests if you want it to be executed in that context. Keep in mind is not 100% deterministic to find problem if fix is not present, would have to be even slower for that.

picnixz · 2025-01-17T14:23:15Z

I don't have much time today and tomorrow for in-depth rewview and there is some other stuff to do before reviewing this one, but I'll do it on Sunday or Monday. If I forget, just ping me back (or you can DM me on Discord @ZeroIntensity)

Lib/test/test_hashlib.py

picnixz

First round of review I forgot to send. I'll have more time starting on Monday to do the in-depth review.

Lib/test/test_hashlib.py

tom-pytel · 2025-01-18T18:37:22Z

Sent up requested changes, but I want to be clear, with or without the tsan fix, this test doesn't actually do anything useful (I have no tsan output or test failure when fix is removed).

$ ./python -m test --tsan test_hashlib -v
== CPython 3.14.0a4+ experimental free-threading build (heads/array-10-g40a4d88a14-dirty:40a4d88a14, Jan 16 2025, 11:58:58) [GCC 11.4.0]
== Linux-6.8.0-51-generic-x86_64-with-glibc2.35 little-endian
== Python build: free_threading debug TSAN
...
test_py_digest_by_name_data_race (test.test_hashlib.KDFTests.test_py_digest_by_name_data_race) ... ok
...
Result: SUCCESS

This catches the race in `py_digest_by_name` that is fixed separately in pythongh-128886.

colesbury

LGTM!

colesbury · 2025-02-07T23:04:42Z

I'll merge this next week if nobody else has further feedback or merges it before me.

gpshead

a little tricky but seems solid.

miss-islington-app · 2025-02-08T02:47:48Z

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

miss-islington-app · 2025-02-08T02:47:52Z

Sorry, @tom-pytel and @gpshead, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 6c67904e793828d84716a8c83436c9495235f3a1 3.13

gpshead · 2025-02-08T02:48:15Z

@colesbury can you handle the backport?

ZeroIntensity · 2025-02-08T02:50:22Z

I think it would also be great to give @tom-pytel a chance to backport if he's up for it.

(cherry picked from commit 6c67904) Co-authored-by: Tomasz Pytel <tompytel@gmail.com>

bedevere-app · 2025-02-08T13:06:08Z

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

tom-pytel · 2025-02-08T13:07:41Z

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

cherry_picker was giving me an error from git so I did this manually from the dry run, feel free to beat with stupid stick if I messes something up. Otherwise let me know and will do same for 3.12.

bedevere-app · 2025-02-08T13:11:39Z

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

ZeroIntensity · 2025-02-08T13:13:44Z

Yeah, you're supposed to get an error for git. That error is telling you to fix the merge conflicts 😅

No need for 3.12, free-threading doesn't exist there.

tom-pytel · 2025-02-08T13:15:24Z

Yeah, you're supposed to get an error for git. That error is telling you to fix the merge conflicts 😅

No need for 3.12, free-threading doesn't exist there.

No, it wasn't that, it was something weird git-related, am gonna post this as an issue unless you have an idea:

(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {main} $ cherry_picker 6c67904e793828d84716a8c83436c9495235f3a1 3.13
🐍 🍒 ⛏
Now backporting '6c67904e793828d84716a8c83436c9495235f3a1' into '3.13'
Error cherry-pick 6c67904e793828d84716a8c83436c9495235f3a1.
Auto-merging Modules/_hashopenssl.c
CONFLICT (content): Merge conflict in Modules/_hashopenssl.c
error: could not apply 6c67904e793... gh-128657: fix _hashopenssl ref/data race (GH-128886)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".


Failed to cherry-pick 6c67904e793828d84716a8c83436c9495235f3a1 into 3.13 ☹
... Stopping here.

To continue and resolve the conflict:
    $ cherry_picker --status  # to find out which files need attention
    # Fix the conflict
    $ cherry_picker --status  # should now say 'all conflict fixed'
    $ cherry_picker --continue

To abort the cherry-pick and cleanup:
    $ cherry_picker --abort


Fix...


(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {backport-6c67904-3.13} $ cherry_picker --status
🐍 🍒 ⛏
On branch backport-6c67904-3.13
You are currently cherry-picking commit 6c67904e793.
  (all conflicts fixed: run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	new file:   Misc/NEWS.d/next/Library/2025-01-15-15-45-21.gh-issue-128657.P5LNQA.rst
	modified:   Modules/_hashopenssl.c


(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {backport-6c67904-3.13} $ cherry_picker --continue
🐍 🍒 ⛏
Traceback (most recent call last):
  File "/home/tom/work/cpython/128657/local/venv/bin/cherry_picker", line 8, in <module>
    sys.exit(cherry_pick_cli())
             ~~~~~~~~~~~~~~~^^
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/click/core.py", line 1161, in __call__
    return self.main(*args, **kwargs)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/click/core.py", line 1082, in main
    rv = self.invoke(ctx)
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/click/core.py", line 1443, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/click/core.py", line 788, in invoke
    return __callback(*args, **kwargs)
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/cherry_picker/cherry_picker.py", line 855, in cherry_pick_cli
    cherry_picker.continue_cherry_pick()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/cherry_picker/cherry_picker.py", line 639, in continue_cherry_pick
    commits = get_commits_from_backport_branch(base)
  File "/home/tom/work/cpython/128657/local/venv/lib/python3.14/site-packages/cherry_picker/cherry_picker.py", line 965, in get_commits_from_backport_branch
    output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
  File "/home/tom/work/cpython/128657/local/lib/python3.14/subprocess.py", line 474, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
           ~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
               **kwargs).stdout
               ^^^^^^^^^
  File "/home/tom/work/cpython/128657/local/lib/python3.14/subprocess.py", line 579, in run
    raise CalledProcessError(retcode, process.args,
                             output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['git', 'log', '--format=%H', '3.13..']' returned non-zero exit status 128.

(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {backport-6c67904-3.13} $ git log --format=%H 3.13..
fatal: ambiguous argument '3.13..': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {backport-6c67904-3.13} $ git --version
git version 2.34.1

(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {backport-6c67904-3.13} $ cherry_picker status
🐍 🍒 ⛏
Run state cherry-picker.state=CONTINUATION_STARTED in Git config is not known.
Perhaps it has been set by a newer version of cherry-picker. Try upgrading.
Valid states are: BACKPORT_PAUSED, UNSET. If this looks suspicious, raise an issue at https://github.com/python/cherry-picker/issues/new.
As the last resort you can reset the runtime state stored in Git config using the following command: `git config --local --remove-section cherry-picker`

ZeroIntensity · 2025-02-08T13:17:22Z

IIRC, that shows up if you don't have a local 3.13 branch from upstream.

tom-pytel · 2025-02-08T13:19:32Z

IIRC, that shows up if you don't have a local 3.13 branch from upstream.

But cherry picker created the branch, and:

(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {backport-6c67904-3.13} $ git remote -v
origin	git@github.com:tom-pytel/cpython.git (fetch)
origin	git@github.com:tom-pytel/cpython.git (push)
upstream	git@github.com:python/cpython.git (fetch)
upstream	git@github.com:python/cpython.git (push)

Or did I have to specify upstream on the command line?

cherry_picker 6c67904e793828d84716a8c83436c9495235f3a1 3.13

It was a branch from upstream/3.13:

(venv) tom@tom-VirtualBox:~/work/cpython/128657/cp {backport-6c67904-3.13} $ git log --all --decorate --oneline --graph
* 610aa38e9bf (HEAD -> backport-6c67904-3.13) [3.13] gh-128657: fix _hashopenssl ref/data race (GH-128886) (cherry picked from commit 6c67904e793828d84716a8c83436c9495235f3a1)
* 8a7146c5eb3 (upstream/3.13) [3.13] gh-117657: Fix data race in `dict_dict_merge` (gh-129755) (gh-129808)
* aae0a1f9044 [3.13] Add multinomial to the itertools recipes docs (gh-129760) (gh-129762)
* f7af8bc58aa  [3.13] gh-129533: Update PyGC_Enable/Disable/IsEnabled to use atomic operat… (gh-129756)
* f7cc8623457 [3.13] gh-129732: Fix race on `shared->array` in qsbr code under free-threading (gh-129738) (gh-129747)

NM, I understand what u mean...

tom-pytel · 2025-02-08T13:40:12Z

Correct backport is here #129853

(cherry picked from commit 6c67904)

* gh-128657: Run test_hashlib with `--parallel-threads` This catches the race in `py_digest_by_name` that is fixed separately in gh-128886. * Adjust assertion order

pythongh-128657: fix _hashopenssl ref/data race

a3215e1

bedevere-app bot mentioned this pull request Jan 15, 2025

Race in py_digest_by_name, _hashopenssl.c under free-threading #128657

Closed

bedevere-app bot added the awaiting review label Jan 15, 2025

blurb-it bot and others added 3 commits January 15, 2025 15:45

📜🤖 Added by blurb_it.

b91ba0b

Update 2025-01-15-15-45-21.gh-issue-128657.P5LNQA.rst

519388d

Update 2025-01-15-15-45-21.gh-issue-128657.P5LNQA.rst

2767d89

ZeroIntensity requested a review from picnixz January 16, 2025 01:21

ZeroIntensity added topic-free-threading needs backport to 3.13 bugs and security fixes labels Jan 16, 2025

ZeroIntensity reviewed Jan 16, 2025

View reviewed changes

Misc/NEWS.d/next/Library/2025-01-15-15-45-21.gh-issue-128657.P5LNQA.rst Outdated Show resolved Hide resolved

Modules/_hashopenssl.c Show resolved Hide resolved

ifdef Py_GIL_DISABLED, change news

4563481

ZeroIntensity reviewed Jan 16, 2025

View reviewed changes

Modules/_hashopenssl.c Outdated Show resolved Hide resolved

tom-pytel added 2 commits January 16, 2025 08:23

add newline to news entry

ccc44df

nit picked

01f498e

tom-pytel requested review from gpshead and tiran as code owners January 16, 2025 16:51

added test and test_hashlib to list of tsan tests

35fd4ba

gpshead reviewed Jan 18, 2025

View reviewed changes

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

ZeroIntensity reviewed Jan 18, 2025

View reviewed changes

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

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

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

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

picnixz reviewed Jan 18, 2025

View reviewed changes

requested changes

c7c497c

colesbury added a commit to colesbury/cpython that referenced this pull request Feb 7, 2025

pythongh-128657: Run test_hashlib with --parallel-threads

ec889f0

This catches the race in `py_digest_by_name` that is fixed separately in pythongh-128886.

colesbury mentioned this pull request Feb 7, 2025

gh-128657: Run test_hashlib with --parallel-threads #129833

Merged

tom-pytel added 2 commits February 7, 2025 17:35

revert tests

1bd904f

Merge branch 'main' into fix-issue-128657

99daec3

colesbury approved these changes Feb 7, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Feb 7, 2025

gpshead approved these changes Feb 8, 2025

View reviewed changes

gpshead merged commit 6c67904 into python:main Feb 8, 2025
46 checks passed

bedevere-app bot removed the awaiting merge label Feb 8, 2025

miss-islington-app bot assigned gpshead Feb 8, 2025

tom-pytel added a commit to tom-pytel/cpython that referenced this pull request Feb 8, 2025

[3.13] pythongh-128657: fix _hashopenssl ref/data race (pythonGH-128886)

610aa38

(cherry picked from commit 6c67904) Co-authored-by: Tomasz Pytel <tompytel@gmail.com>

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Feb 8, 2025

colesbury pushed a commit that referenced this pull request Feb 8, 2025

[3.13] gh-128657: fix _hashopenssl ref/data race (GH-128886) (GH-129853)

4a1389c

(cherry picked from commit 6c67904)

tom-pytel deleted the fix-issue-128657 branch March 4, 2025 12:02

picnixz mentioned this pull request Apr 18, 2025

Compiler warnings on free-threaded build for _hashopenssl.c #132674

Closed

Uh oh!

Conversation

tom-pytel commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZeroIntensity commented Jan 16, 2025

Uh oh!

tom-pytel commented Jan 16, 2025

Uh oh!

ZeroIntensity commented Jan 16, 2025

Uh oh!

tom-pytel commented Jan 16, 2025

Uh oh!

ZeroIntensity commented Jan 16, 2025

Uh oh!

tom-pytel commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz commented Jan 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tom-pytel commented Jan 18, 2025

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

colesbury commented Feb 7, 2025

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app bot commented Feb 8, 2025

Uh oh!

miss-islington-app bot commented Feb 8, 2025

Uh oh!

gpshead commented Feb 8, 2025

Uh oh!

ZeroIntensity commented Feb 8, 2025

Uh oh!

bedevere-app bot commented Feb 8, 2025

Uh oh!

tom-pytel commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Feb 8, 2025

Uh oh!

ZeroIntensity commented Feb 8, 2025

Uh oh!

tom-pytel commented Feb 8, 2025

Uh oh!

ZeroIntensity commented Feb 8, 2025

Uh oh!

tom-pytel commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tom-pytel commented Feb 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tom-pytel commented Jan 15, 2025 •

edited

Loading

tom-pytel commented Jan 16, 2025 •

edited

Loading

tom-pytel commented Feb 8, 2025 •

edited

Loading

tom-pytel commented Feb 8, 2025 •

edited

Loading