gh-128657: fix _hashopenssl ref/data race #128886

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.

ZeroIntensity · 2025-01-16T01:27:38Z

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

@@ -0,0 +1 @@
+Fix a possible data and PY_EVP_MD refcount race in _hashopenssl.c py_digest_by_name() under free-threading.


In general, we try to avoid technical details in the changelog. How about something like this?

Suggested change

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

Fix a crash when using objects returned by :func:`hashlib.sha256` under :term:`free threading`.

Done (though not sure how to state this less technically), but just to be clear there is no crash. All this patch does is prevent a possible extra erroneous refcount on PY_EVP_MD under free threading (and shuts up TSAN).

Modules/_hashopenssl.c

ZeroIntensity · 2025-01-16T13:21:16Z

Modules/_hashopenssl.c

@@ -26,6 +26,7 @@
 #include "Python.h"
 #include "pycore_hashtable.h"
 #include "pycore_strhex.h"        // _Py_strhex()


Tiny nitpick: let's align this comment with the new one below it.

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)

gpshead · 2025-01-18T00:38:07Z

Lib/test/test_hashlib.py

@@ -1196,6 +1196,29 @@ def test_file_digest(self):
            with open(os_helper.TESTFN, "wb") as f:
                hashlib.file_digest(f, "sha256")

+    @unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")
+    def test_gh_128657(self):


A test that doesn't assert anything always looks suspicious. I think you're hoping for this to trigger TSAN issues and thus this isn't really a pass/fail on its own. Just summarize that kind of thing in a comment within the test for later people who look at this code.

I am not really sure the test should be there. If the criteria is that any fixed tsan issue should get a test then pretty soon the tsan section will be full of tests that don't really do anything except run for longer and longer times?

We do need to (technically) assert something--the thread shouldn't raise exceptions, and that's what the test is for if TSan is disabled.

ZeroIntensity · 2025-01-18T17:04:44Z

Lib/test/test_hashlib.py

@@ -1196,6 +1196,29 @@ def test_file_digest(self):
            with open(os_helper.TESTFN, "wb") as f:
                hashlib.file_digest(f, "sha256")

+    @unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")


IIRC, TSan is run for the default build as well, because things like subinterpreters can have thread safety issues.

I think it's worth always running this for FT, because we'll need to assert that the thread doesn't raise exceptions (more on that in another comment).

Should only run if threading actually works.

Suggested change

@unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")

@unittest.skipUnless(support.Py_GIL_DISABLED, "only meaningful on free-threading")

@threading_helper.requires_working_threading()

ZeroIntensity · 2025-01-18T17:06:22Z

Lib/test/test_hashlib.py

+            thrds = []
+
+            for i in range(num_workers):
+                thrds.append(thrd := threading.Thread(target=closure, args=(barrier,)))
+                thrd.start()
+
+            for thrd in thrds:
+                thrd.join()


We have a utility for starting threads that will simplify this, and we also need to make sure that the thread doesn't raise an exception (threads that fail will not propagate an error to the main thread).

Suggested change

thrds = []

for i in range(num_workers):

thrds.append(thrd := threading.Thread(target=closure, args=(barrier,)))

thrd.start()

for thrd in thrds:

thrd.join()

with threading_helper.catch_threading_exception() as cm:

thrds = [threading.Thread(target=closure, args=(barrier,))]

with threading_helper.start_threads(thrds):

pass

if cm.exc_value:

raise cm.exc_value

ZeroIntensity · 2025-01-18T17:14:19Z

Lib/test/test_hashlib.py

@@ -1196,6 +1196,29 @@ def test_file_digest(self):
            with open(os_helper.TESTFN, "wb") as f:
                hashlib.file_digest(f, "sha256")

+    @unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")
+    def test_gh_128657(self):


We do need to (technically) assert something--the thread shouldn't raise exceptions, and that's what the test is for if TSan is disabled.

ZeroIntensity · 2025-01-18T17:15:00Z

Lib/test/test_hashlib.py

+            test_hashlib_sha256()
+
+        num_workers = 40
+        num_runs = 20


I don't think we need num_runs. I haven't used or seen it in any other free-threading PRs.

picnixz

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

picnixz · 2025-01-17T18:52:28Z

Lib/test/test_hashlib.py

@@ -1196,6 +1196,29 @@ def test_file_digest(self):
            with open(os_helper.TESTFN, "wb") as f:
                hashlib.file_digest(f, "sha256")

+    @unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")


Suggested change

@unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")

@unittest.skipUnless(support.check_sanitizer(thread=True),

"only meaningful on free-threading")

picnixz · 2025-01-17T18:53:07Z

Lib/test/test_hashlib.py

@@ -1196,6 +1196,29 @@ def test_file_digest(self):
            with open(os_helper.TESTFN, "wb") as f:
                hashlib.file_digest(f, "sha256")

+    @unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")
+    def test_gh_128657(self):


Can we have a better test case name please as well as a small comment (not docstring) to explain what this test is meant to assert (and then the reference to the GH issue)? this does not help to know why this test is here unless we open GH.

picnixz · 2025-01-17T18:53:47Z

Lib/test/test_hashlib.py

+            thrds = []
+
+            for i in range(num_workers):
+                thrds.append(thrd := threading.Thread(target=closure, args=(barrier,)))


Suggested change

thrds.append(thrd := threading.Thread(target=closure, args=(barrier,)))

thread = threading.Thread(target=closure, args=(barrier,))

threads.append(thread)

picnixz · 2025-01-17T18:53:54Z

Lib/test/test_hashlib.py

+
+        for i in range(num_runs):
+            barrier = threading.Barrier(num_workers)
+            thrds = []


Suggested change

thrds = []

threads = []

picnixz · 2025-01-17T18:54:31Z

Lib/test/test_hashlib.py

+                thrd.start()
+
+            for thrd in thrds:
+                thrd.join()


Suggested change

thrd.start()

for thrd in thrds:

thrd.join()

thread.start()

for thread in threads:

thread.join()

I would prefer full names. Or if you want short names, you can use t for the thread variable (but I would prefer to keep threads).

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

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

Open

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

ifdef Py_GIL_DISABLED, change news

4563481

ZeroIntensity reviewed Jan 16, 2025

View reviewed changes

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

ZeroIntensity reviewed Jan 18, 2025

View reviewed changes

picnixz reviewed Jan 18, 2025

View reviewed changes

requested changes

c7c497c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-128657: fix _hashopenssl ref/data race #128886

gh-128657: fix _hashopenssl ref/data race #128886

tom-pytel commented Jan 15, 2025 •

edited

Loading

bedevere-app bot commented Jan 15, 2025

ZeroIntensity Jan 16, 2025

tom-pytel Jan 16, 2025

ZeroIntensity Jan 16, 2025 •

edited

Loading

ZeroIntensity commented Jan 16, 2025

tom-pytel commented Jan 16, 2025

ZeroIntensity commented Jan 16, 2025

tom-pytel commented Jan 16, 2025

ZeroIntensity commented Jan 16, 2025

tom-pytel commented Jan 16, 2025 •

edited

Loading

picnixz commented Jan 17, 2025

gpshead Jan 18, 2025

tom-pytel Jan 18, 2025

ZeroIntensity Jan 18, 2025

ZeroIntensity Jan 18, 2025

ZeroIntensity Jan 18, 2025

ZeroIntensity Jan 18, 2025

ZeroIntensity Jan 18, 2025

picnixz left a comment

picnixz Jan 17, 2025

picnixz Jan 17, 2025

picnixz Jan 17, 2025

picnixz Jan 17, 2025

picnixz Jan 17, 2025

tom-pytel commented Jan 18, 2025

		@@ -0,0 +1 @@
		Fix a possible data and PY_EVP_MD refcount race in _hashopenssl.c py_digest_by_name() under free-threading.

	Fix a possible data and PY_EVP_MD refcount race in _hashopenssl.c py_digest_by_name() under free-threading.
	Fix a crash when using objects returned by :func:`hashlib.sha256` under :term:`free threading`.

	@unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")
	@unittest.skipUnless(support.Py_GIL_DISABLED, "only meaningful on free-threading")
	@threading_helper.requires_working_threading()

	@unittest.skipUnless(support.check_sanitizer(thread=True), "only meaningful on free-threading")
	@unittest.skipUnless(support.check_sanitizer(thread=True),
	"only meaningful on free-threading")

	thrds.append(thrd := threading.Thread(target=closure, args=(barrier,)))
	thread = threading.Thread(target=closure, args=(barrier,))
	threads.append(thread)

gh-128657: fix _hashopenssl ref/data race #128886

Are you sure you want to change the base?

gh-128657: fix _hashopenssl ref/data race #128886

Conversation

tom-pytel commented Jan 15, 2025 • edited Loading

bedevere-app bot commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroIntensity Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

ZeroIntensity commented Jan 16, 2025

tom-pytel commented Jan 16, 2025

ZeroIntensity commented Jan 16, 2025

tom-pytel commented Jan 16, 2025

ZeroIntensity commented Jan 16, 2025

tom-pytel commented Jan 16, 2025 • edited Loading

picnixz commented Jan 17, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-pytel commented Jan 18, 2025

tom-pytel commented Jan 15, 2025 •

edited

Loading

ZeroIntensity Jan 16, 2025 •

edited

Loading

tom-pytel commented Jan 16, 2025 •

edited

Loading