Skip to content

gh-145301: Fix double-free in hashlib and hmac module initialization - #145321

Merged
gpshead merged 1 commit into
python:mainfrom
krylosov-aa:fix-issue-145301
Mar 5, 2026
Merged

gh-145301: Fix double-free in hashlib and hmac module initialization#145321
gpshead merged 1 commit into
python:mainfrom
krylosov-aa:fix-issue-145301

Conversation

@krylosov-aa

@krylosov-aakrylosov-aa commented Feb 27, 2026

Copy link
Copy Markdown
Contributor

Both hashlib and hmac modules have similar double-free bugs in their hashtable initialization code.

Problem

hashlib

In py_hashentry_table_new(), when _Py_hashtable_set() fails while adding an entry by its py_alias key (after successfully adding it by py_name), the code calls PyMem_Free(entry) before goto error. The error handler then calls _Py_hashtable_destroy() which frees the same entry again via py_hashentry_t_destroy_value().

hmac

In py_hmac_hinfo_ht_new(), the Py_HMAC_HINFO_LINK macro has the same issue. When py_hmac_hinfo_ht_add() fails for hashlib_name key (after successfully adding name), the code calls PyMem_Free(value) while the entry is already in the hashtable with refcnt=1.

Solution

hashlib

Remove the manual PyMem_Free(entry) call since the entry is already tracked by the hashtable under the py_name key and will be properly freed by _Py_hashtable_destroy().

hmac

Add a refcnt check before freeing: only call PyMem_Free(value) if value->refcnt == 0 (meaning it was never added to the table). If refcnt > 0, the entry is already in the table and will be freed by _Py_hashtable_destroy().

Tests

No test added. I couldn't find a practical way to trigger this bug since it requires _Py_hashtable_set() to fail during module initialization. This type of bug is best caught by sanitizers rather than unit tests.

The fix can be verified by running the reproducer provided by the issue author:

Build with ASan:

mkdir build-asan &&cd build-asan
../configure --with-pydebug --with-address-sanitizer --without-pymalloc
make -j$(nproc)

Reproducer script (uaf_asan.py):

importsubprocessimportsyscode= (
"import sys, _testcapi\n""if '_hashlib' in sys.modules:\n"" del sys.modules['_hashlib']\n""_testcapi.set_nomemory(40, 41)\n""try:\n"" import _hashlib\n""except (MemoryError, ImportError):\n"" pass\n""finally:\n"" _testcapi.remove_mem_hooks()\n"
)
result=subprocess.run(
[sys.executable, '-c', code],
capture_output=True, text=True, timeout=10
)
ifresult.returncode!=0:
print(f"[*] CRASH confirmed (rc={result.returncode})")
print(f"[*] {result.stderr.strip().split(chr(10))[-1]}")
else:
print("[*] No crash (try different start values)")

Before fix:

$ ./build-asan/python uaf_asan.py
[*] CRASH confirmed (rc=-6)
[*] python: ../Include/internal/pycore_stackref.h:554: PyStackRef_FromPyObjectSteal: Assertion `obj != NULL' failed.

After fix: No crash.

@python-cla-bot

python-cla-botBot commented Feb 27, 2026

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

Comment on lines +1 to +3
Fix a double-free bug in :mod:`_hashlib` module initialization when
``_Py_hashtable_set()`` fails while adding an algorithm alias to the hash
table after the primary name was already added

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too verbose:

Suggested change
Fix a double-free bug in :mod:`_hashlib` module initialization when
``_Py_hashtable_set()`` fails while adding an algorithm alias to the hash
table after the primary name was already added
:mod:`hashlib`: fix a crash when the C extension module initialization fails.

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the test repro as a regression test.

@bedevere-app

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@picnixzpicnixz added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Feb 27, 2026
@picnixz

Copy link
Copy Markdown
Member

Can you also check whether _hmac suffers from that? I don't remember whether I actually C/Ced this code or if I used another refcounting approach.

@krylosov-aa

Copy link
Copy Markdown
ContributorAuthor

Can you also check whether _hmac suffers from that? I don't remember whether I actually C/Ced this code or if I used another refcounting approach.

I checked the HMAC implementation in _hashopenssl.c. It uses the same state->hashtable that is initialized by py_hashentry_table_new(). There's no separate hashtable for HMAC. So this fix covers both hash functions and HMAC.

@picnixz

Copy link
Copy Markdown
Member

I'm sorry, I meant the hmacmodule.c

@krylosov-aa

krylosov-aa commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

I'm sorry, I meant the hmacmodule.c

Yes, hmacmodule.c has the same issue in py_hmac_hinfo_ht_new().

I can suggest the following solution:

#define Py_HMAC_HINFO_LINK(KEY) \
do { \
int rc = py_hmac_hinfo_ht_add(table, KEY, value); \
if (rc < 0) { \
+ if (value->refcnt == 0) { \
PyMem_Free(value); \
+ } \
goto error; \
} \
else if (rc == 1) { \
value->refcnt++; \
} \
} while (0)
Py_HMAC_HINFO_LINK(e->name);
Py_HMAC_HINFO_LINK(e->hashlib_name);
#undef Py_HMAC_HINFO_LINK

Should I include this fix in the same PR or create a separate one?

@picnixz

Copy link
Copy Markdown
Member

Yes, please do so, and put a comment as well.

@krylosov-aa

krylosov-aa commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

Please add the test repro as a regression test.

I investigated adding a regression test using _testcapi.set_nomemory, but found it's not feasible for this specific bug.

The set_nomemory() approach from the issue report triggers allocation failures globally, and the exact allocation number that hits our specific code path varies by platform/configuration.

Do you have any suggestions for how to reliably test this?

@krylosov-aakrylosov-aa changed the title gh-145301: Fix double-free in _hashlib module initializationgh-145301: Fix double-free in hashlib and hmac module initializationFeb 27, 2026
Comment threadModules/hmacmodule.c Outdated
int rc = py_hmac_hinfo_ht_add(table, KEY, value); \
if (rc < 0) { \
PyMem_Free(value); \
/* entry may already be in ht, will be freed by _Py_hashtable_destroy() */ \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* entry may already be in ht, will be freed by _Py_hashtable_destroy() */ \
/*entrymayalreadybeinht, willbe\
*freedby_Py_hashtable_destroy() */ \
  • correctly align the \. You can augment the column if it does not exceed 80 chars. If it does, properly wrap the comment. Tia.

@picnixz

Copy link
Copy Markdown
Member

Do you have any suggestions for how to reliably test this?

You have an ASASN reproducer, why not use it?

Comment threadLib/test/test_hashlib.py Outdated
import tempfile
import threading
import unittest
import subprocess

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the imports alphabetically sorted.

Comment threadLib/test/test_hashlib.py Outdated
Comment on lines +1210 to +1215
try:
import _testcapi
if not hasattr(_testcapi, 'set_nomemory'):
self.skipTest('requires _testcapi.set_nomemory')
except ImportError:
self.skipTest('requires _testcapi')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use this, use import_helper for that.

Comment threadLib/test/test_hashlib.py Outdated
Comment on lines +1221 to +1222
if '_hashlib' in sys.modules:
del sys.modules['_hashlib']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary.

Comment threadLib/test/test_hashlib.py Outdated
_testcapi.remove_mem_hooks()
""")

rc = subprocess.call(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use assert_python_ok instead.

@@ -0,0 +1 @@
Fix a crash when :mod:`hashlib` or :mod:`hmac` C extension module initialization fails.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create two NEWS entries, one for hashlib and for hmac please with the same sentence:

:mod:`<name>`: fix a crash when the initialization of the underlying C extension module fails.

Comment threadModules/hmacmodule.c Outdated
Comment on lines +1460 to +1461
/* entry may already be in ht, will be freed by \
_Py_hashtable_destroy() */ \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just put re-align the \ directly and put the comment on one line (you can remove the "will be freed by _Py_hashtable_destroy() and mention that it will be freed "upon exit").

Comment threadLib/test/test_hashlib.py Outdated
self.skipTest('requires _testcapi')

code = textwrap.dedent("""
import sys

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sys is no more necessary

Comment threadLib/test/test_hashlib.py Outdated

requires_sha3 = unittest.skipUnless(_sha3, 'requires _sha3')

_testcapi = import_helper.import_module('_testcapi')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do it at at the module level, it will skip the entire test module. Do it inside the test directly.

Comment threadLib/test/test_hashlib.py Outdated
hash_type.value = False

@unittest.skipUnless(HASH is not None, 'need _hashlib')
@unittest.skipUnless(hasattr(_testcapi, 'set_nomemory'),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is _testcapi lacking this function?

@krylosov-aa

Copy link
Copy Markdown
ContributorAuthor

Do you have any suggestions for how to reliably test this?

You have an ASASN reproducer, why not use it?

I think that testing using set_nomemory(40, 41) is a bad idea because the number of memory allocations differs across platforms, and we cannot guarantee that allocations 40-41 will always occur in py_hashentry_table_new on all platforms.

I believe this is why the macOS Intel test crashes.

I don't know how to write a regression test for this scenario. I would suggest removing the test, since the fix is simple, comments have been left, and a regression seems unlikely to happen.

Comment threadLib/test/test_hashlib.py Outdated
import_helper.import_module('_testcapi')
code = textwrap.dedent("""
import _testcapi
_testcapi.set_nomemory(40, 41)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this seems flaky. It will change depending on the python's version and a platform.

Comment threadLib/test/test_hashlib.py Outdated
from test.support import _4G, bigmemtest
from test.support import hashlib_helper
from test.support.import_helper import import_fresh_module
from test.support import import_helper

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we please revert this change if you decide to remove the test?

Comment threadModules/hmacmodule.c Outdated
Comment threadModules/hmacmodule.c Outdated
@picnixz

Copy link
Copy Markdown
Member

Oo for removig the test.

@bedevere-app

Copy link
Copy Markdown

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-app
bedevere-appBot requested a review from picnixzMarch 2, 2026 21:23

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

But I'm not sure that the NEWS entries are worth it since it's unlikely to get a memory allocation failure on importing these C extensions.

@gpshead
gpshead merged commit 6acaf65 into python:mainMar 5, 2026
52 of 55 checks passed
@miss-islington-app

Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 5, 2026
…ation (pythonGH-145321)
(cherry picked from commit 6acaf65)
Co-authored-by: krylosov-aa <krylosov.andrew@gmail.com>
pythongh-145301: Fix double-free in hashlib and hmac initialization
@miss-islington-app

Copy link
Copy Markdown

Sorry, @krylosov-aa 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 6acaf659ef0fdee131bc02f0b58685da039b5855 3.13

@bedevere-app

Copy link
Copy Markdown

GH-145523 is a backport of this pull request to the 3.14 branch.

@bedevere-appbedevere-appBot removed the needs backport to 3.14 bugs and security fixes label Mar 5, 2026
gpshead pushed a commit that referenced this pull request Mar 5, 2026
…zation (GH-145321) (#145523)
gh-145301: Fix double-free in hashlib and hmac module initialization (GH-145321)
(cherry picked from commit 6acaf65)
gh-145301: Fix double-free in hashlib and hmac initialization
Co-authored-by: krylosov-aa <krylosov.andrew@gmail.com>
@bedevere-bot

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD14 3.14 (tier-3) has failed when building commit f426928.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1740/builds/1059) and take a look at the build logs.
  4. Check if the failure is related to this commit (f426928) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1740/builds/1059

Failed tests:

  • test_httpservers

Failed subtests:

  • test_large_content_length_truncated - test.test_httpservers.CGIHTTPServerTestCase.test_large_content_length_truncated

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/test/test_httpservers.py", line 1139, in test_large_content_length_truncated
res =self.request('/cgi-bin/file1.py', 'POST', b'x', headers)
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/test/test_httpservers.py", line 132, in requestreturnself.connection.getresponse()
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/http/client.py", line 1450, in getresponse
response.begin()
~~~~~~~~~~~~~~^^
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/http/client.py", line 336, in begin
version, status, reason =self._read_status()
~~~~~~~~~~~~~~~~~^^
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/http/client.py", line 305, in _read_statusraise RemoteDisconnected("Remote end closed connection without"" response")
http.client.RemoteDisconnected: Remote end closed connection without response

@sergey-miryanov

Copy link
Copy Markdown
Contributor

@krylosov-aa Could you please take care of manually backporting to 3.13?

@krylosov-aa

Copy link
Copy Markdown
ContributorAuthor

@krylosov-aa Could you please take care of manually backporting to 3.13?

Yes, I will do it.

@picnixz

Copy link
Copy Markdown
Member

For 3.13, it is only hashopenssl that you need to change. HMAC HACL* did not exist yet.

@vstinner

Copy link
Copy Markdown
Member

This is a nice tricky fix :-) Thanks @krylosov-aa for the fix and thanks @raminfp for the bug report!

@bedevere-app

Copy link
Copy Markdown

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

@bedevere-appbedevere-appBot removed the needs backport to 3.13 bugs and security fixes label Mar 5, 2026
@krylosov-aa

Copy link
Copy Markdown
ContributorAuthor

@picnixz@sergey-miryanov

I have created a manual backport for 3.13: see #145532

@bedevere-app

Copy link
Copy Markdown

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

encukou pushed a commit that referenced this pull request Mar 10, 2026
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@krylosov-aa@picnixz@bedevere-bot@sergey-miryanov@vstinner@gpshead@sobolevn