gh-149816: fix a race when fetching OpenSSL digests

[picnixz]

@devdanzin Your didn't include a reproducer for this issue so could you actually check whether this fix would be suitable. I'm not a FT expert but the problem was essentially a race ref leak right?

• Issue: 22 free-threading race conditions #149816

[devdanzin]

That's not one of mine, but I had a go at reproducing it. Needed an instrumented build to log leaks, as neither TSan, LSan or ASan could see the leak. The leak reproduces without the fix and stops with the fix applied, that should be enough confirmation.

Claude notes: "the old code had a second, latent bug the fix also closes — in the (always-taken) same-pointer case the losing thread skipped up_ref, so its caller received a digest with no reference added (borrowed-as-owned → potential over-free/UAF if the cache entry were ever cleared). The fix's unconditional up_ref fixes that too.".

Below is the patch, reproducer for patched build and README that Claude came up with to test the fix.

---

diff --git a/Modules/_hashopenssl.c b/Modules/_hashopenssl.c
index f895c903748..434c3ff231c 100644
--- a/Modules/_hashopenssl.c+++ b/Modules/_hashopenssl.c@@ -688,6 +688,22 @@ disable_fips_property(Py_hash_type py_ht)
*
* If 'name' is an OpenSSL indexed name, the return value is cached.
*/
+#ifdef HASHLIB_EVP_RACE_COUNTER+/*+ * Opt-in diagnostic counter for gh-149816 finding (21) / gh-153019.+ *+ * Build with CFLAGS=-DHASHLIB_EVP_RACE_COUNTER to enable. Counts the number of+ * threads that fetched a fresh EVP_MD but lost the race to publish it into the+ * per-entry cache. On the buggy (pre-gh-153019) code below, each such event is+ * one LEAKED EVP_MD reference (the overwritten `other_digest` is never freed).+ *+ * Exposed to Python as _hashlib._evp_md_race_events() /+ * _hashlib._evp_md_race_events_reset(). No effect unless the macro is defined,+ * and a no-op on GIL-enabled builds (the race cannot occur there).+ */+static int _evp_md_race_events = 0;+#endif+
static PY_EVP_MD *
get_openssl_evp_md_by_utf8name(_hashlibstate *state, const char *name,
Py_hash_type py_ht)
@@ -722,6 +738,14 @@ get_openssl_evp_md_by_utf8name(_hashlibstate *state, const char *name,
}
// if another thread same thing at same time make sure we got same ptr
assert(other_digest == NULL || other_digest == digest);
+#ifdef HASHLIB_EVP_RACE_COUNTER+ if (other_digest != NULL) {+ /* Race loser: this thread's freshly fetched digest was overwritten+ * in the cache and its reference (other_digest) is never freed —+ * one leaked EVP_MD reference. (gh-149816 finding 21.) */+ _Py_atomic_add_int(&_evp_md_race_events, 1);+ }+#endif
if (digest != NULL && other_digest == NULL) {
PY_EVP_MD_up_ref(digest);
}
@@ -2682,7 +2706,29 @@ _hashlib_compare_digest_impl(PyObject *module, PyObject *a, PyObject *b)
/* List of functions exported by this module */
+#ifdef HASHLIB_EVP_RACE_COUNTER+static PyObject *+_hashlib_evp_md_race_events(PyObject *module, PyObject *Py_UNUSED(ignored))+{+ return PyLong_FromLong(_Py_atomic_load_int(&_evp_md_race_events));+}++static PyObject *+_hashlib_evp_md_race_events_reset(PyObject *module, PyObject *Py_UNUSED(ignored))+{+ _Py_atomic_store_int(&_evp_md_race_events, 0);+ Py_RETURN_NONE;+}+#endif+
static struct PyMethodDef EVP_functions[] = {
+#ifdef HASHLIB_EVP_RACE_COUNTER+ {"_evp_md_race_events", _hashlib_evp_md_race_events, METH_NOARGS,+ PyDoc_STR("Number of EVP_MD cache publish race-losers observed "+ "(gh-149816 finding 21; each is one leaked ref on the buggy build).")},+ {"_evp_md_race_events_reset", _hashlib_evp_md_race_events_reset, METH_NOARGS,+ PyDoc_STR("Reset the EVP_MD race-loser counter to zero.")},+#endif
_HASHLIB_HASH_NEW_METHODDEF
PBKDF2_HMAC_METHODDEF
_HASHLIB_SCRYPT_METHODDEF

"""Reproducer for gh-149816 finding (21): "Racy EVP_MD cache overwrite leaksreferences in Modules/_hashopenssl.c" — the bug fixed by PR gh-153019.Buggy code (get_openssl_evp_md_by_utf8name, pre-fix): digest = FT_ATOMIC_LOAD_PTR_RELAXED(entry->evp); // slot starts NULL if (digest == NULL) { digest = PY_EVP_MD_fetch(entry->ossl_name, NULL); // slow fetch, +1 other_digest = _Py_atomic_exchange_ptr(&entry->evp, digest); } ... assert(other_digest == NULL || other_digest == digest); if (digest != NULL && other_digest == NULL) PY_EVP_MD_up_ref(digest);The race window is the whole EVP_MD_fetch() call. If several threads hit theFIRST use of an uncached digest simultaneously, they all read the slot as NULL,all fetch, and all atomic-exchange. Every store past the first overwrites (andleaks) the previous thread's freshly fetched digest: `other_digest` is thatextra reference, and it is never freed.OpenSSL's method store makes EVP_MD_fetch return the *same* refcounted pointeracross threads, so `other_digest == digest` (the assert never fires) and theleak is a refcount inflation on a still-reachable object — invisible to TSan,ASan, and LSan. That is why this finding had no reproducer. To observe it, build_hashopenssl.c with -DHASHLIB_EVP_RACE_COUNTER (see hashlib_evp_race_counter.patch),which exposes _hashlib._evp_md_race_events().Requires: free-threaded build, OpenSSL 3.0+. Under PYTHON_GIL=1 / -X gil=1 therace cannot occur and the counter stays 0.Usage: ./python repro_ft_hashopenssl_race.py # free-threaded: race events > 0 ./python -X gil=1 repro_ft_hashopenssl_race.py # serialized: race events == 0"""importsysimportthreadingimport_hashlib# use the C module directly; don't let hashlib pre-touch slots# Names backed by entries in _hashopenssl.c's py_hashes[] table (each has two# lazily-filled cache slots: evp [usedforsecurity=True] and evp_nosecurity).NAMES= [
"md5", "sha1", "sha224", "sha256", "sha384", "sha512",
"sha512_224", "sha512_256",
"sha3_224", "sha3_256", "sha3_384", "sha3_512",
"shake_128", "shake_256", "blake2s", "blake2b",
]
NTHREADS=64HAS_COUNTER=hasattr(_hashlib, "_evp_md_race_events")
print("gil_enabled:", sys._is_gil_enabled(),
"| race counter available:", HAS_COUNTER, flush=True)
# A spin barrier releases all threads within a cache-coherency delay of each# other — far tighter than threading.Barrier's condition-variable wakeup, which# is too staggered relative to the ~microsecond EVP_MD_fetch window.defhammer(name, used_for_security, ready, go):
withready[1]:
ready[0] +=1whilenotgo[0]: # spin until main flips the flag — near-simultaneous releasepasstry:
_hashlib.new(name, b"", usedforsecurity=used_for_security)
exceptValueError:
pass# digest not provided by this OpenSSL builddefmain():
ifHAS_COUNTER:
_hashlib._evp_md_race_events_reset()
windows=0fornameinNAMES:
forufsin (True, False):
go= [False]
ready= [0, threading.Lock()]
ts= [
threading.Thread(target=hammer, args=(name, ufs, ready, go))
for_inrange(NTHREADS)
]
fortints:
t.start()
whileready[0] <NTHREADS: # wait until every thread is spinningpassgo[0] =True# release them all at oncefortints:
t.join()
windows+=1print(f"done: {windows} first-touch race windows", flush=True)
ifHAS_COUNTER:
n=_hashlib._evp_md_race_events()
kind="LEAKED EVP_MD refs"ifnelse"race events"print(f"race-losers: {n} "f"({'BUG: '+str(n) +' '+kindifnelse'none — no leak'})",
flush=True)
# Non-zero on a free-threaded *buggy* build; 0 when serialized or fixed.sys.exit(1ifnelse0)
main()

EVP_MD cache race-leak counter (gh-149816 finding 21 / gh-153019)

A reusable, opt-in instrumentation for demonstrating and verifying the fix for
the racy EVP_MD cache overwrite in Modules/_hashopenssl.c
(get_openssl_evp_md_by_utf8name).

Why it's needed

When two threads first-touch an uncached digest slot on a free-threaded build,
both EVP_MD_fetch() and both publish via _Py_atomic_exchange_ptr. Every store
past the first leaks the overwritten reference (other_digest, never freed).

OpenSSL's method store returns the same refcounted EVP_MD* for concurrent
fetches, so other_digest == digest — the debug assert never fires and the leak
is a refcount inflation on a still-reachable object. That makes it invisible to
TSan, ASan, and LSan, which is why the finding shipped without a reproducer.

This patch adds an atomic counter of race-losers and exposes it to Python, so the
leak is directly observable and countable.

Files

• hashlib_evp_race_counter.patch — gated (#ifdef HASHLIB_EVP_RACE_COUNTER)
  instrumentation of Modules/_hashopenssl.c. Adds a counter, increments it at
  the race-loser site, and exposes _hashlib._evp_md_race_events() /
  _hashlib._evp_md_race_events_reset(). No effect unless the macro is defined.
• repro_ft_hashopenssl_race.py — self-contained stress reproducer. Uses a spin
  barrier under free-threading (tight release into the fetch window) and falls
  back to threading.Barrier under the GIL. Reads the counter if present.

Build (free-threaded, e.g. the matrix debug-ft-nojit-tsan tree)

cd<cpython-ft-tree>
git apply /path/to/hashlib_evp_race_counter.patch
rm -f Modules/_hashopenssl.o
make -j"$(nproc)" CFLAGS_NODIST="-DHASHLIB_EVP_RACE_COUNTER"

Run

# Buggy build (pre-gh-153019), free-threaded -> race-losers > 0, exit 1:
./python repro_ft_hashopenssl_race.py
# race-losers: 1815 (BUG: 1815 LEAKED EVP_MD refs)# Same binary, GIL forced on -> race cannot occur, 0, exit 0:
./python -X gil=1 repro_ft_hashopenssl_race.py
# race-losers: 0 (none — no leak)

Run under TSan with TSAN_OPTIONS="halt_on_error=0 exitcode=0" to also confirm
no data races (the accesses are atomic; there are none in either code shape).

Verifying the fix (gh-153019)

The patch targets the pre-fix code, where the leaked reference is the
other_digest returned by _Py_atomic_exchange_ptr. After applying the fix the
function no longer has other_digest; the equivalent "race-loser" site is the
compare-exchange failure branch. To keep counting on a fixed tree, move the
increment there:

else {
_Py_atomic_add_int(&_evp_md_race_events, 1); // race loser (now freed)PY_EVP_MD_free(fresh); // fixed: loser frees its fetch, no leakdigest=expected;
}

On the fixed build the counter reports the same number of race-losers under
the same contention (~1800), but each one now frees its fetch instead of leaking
— demonstrating the fix handles the identical race with zero leaked references.
./python -m test test_hashlib passes and TSan reports no races.

[picnixz]

On the fixed build the counter reports the same number of race-losers under
the same contention (~1800), but each one now frees its fetch instead of leaking
— demonstrating the fix handles the identical race with zero leaked references.
./python -m test test_hashlib passes and TSan reports no races.

IIUC my fix is also correct so.

[picnixz]

@devdanzin You say that it's not visible under A/TSan, but would it be visible under valgrind? I have other plans for today so I'm not sure I can check that myself but in general, if it's not visible under A/TSan, it should be visible under valgrind I guess?

[devdanzin]

@picnixz I checked (valgrind 3.26.0, memcheck --leak-check=full, on the pre-fix code at main). The answer is no, the reason is that this isn't a tool limitation.

1. Under default valgrind the race doesn't even fire. Its serialized scheduling lets the first thread finish the whole fetch-and-publish before the next thread reads the slot. So a clean valgrind run mostly means "the race didn't happen": I had to shrink the scheduling quantum and use a spin release to get 45 race events to fire inside valgrind at all.

2. Even when the race fires, nothing survives to the leak check: the two halves of the bug cancel. Each race loser orphans the overwritten reference (other_digest, +1), but it also skips PY_EVP_MD_up_ref, so its caller ends up doing an unpaired PY_EVP_MD_free (e.g. _hashlib_HASH_new frees the digest unconditionally on exit, −1). Since concurrent fetches return the same EVP_MD*, the orphan and the unpaired free cancel exactly, and the heap is balanced. There is no net leak at rest, which is why every exit-time detector (LSan, memcheck) sees nothing, ASan sees no invalid access, and TSan sees no non-atomic access.

So I'd refine my earlier "leaked reference" framing: on the pre-fix code the per-event hazard is a transient refcount-imbalance pair, the losing thread's caller runs on a reference it doesn't own (benign today only because cache entries are never replaced until module teardown), and if concurrent fetches ever returned distinct pointers (the case the assert(other_digest == NULL || other_digest == digest) worries about, compiled out in release builds), the loser's caller would over-free the object sitting in the cache. Your fix removes both halves (loser frees its own fetch; up_ref is unconditional), so this doesn't change the conclusion that the fix is correct, it just means no unaugmented tool (TSan, ASan, LSan, or valgrind) will ever show the bug or verify the fix. The counter instrumentation is, as far as I can tell, the only way to observe it.

Investigation and draft by Claude Code.