pythongh-99108: Refactor _sha256 & _sha512 into _sha2.

This merges their code. They're backed by the same single HACL* star static library, having them be a single module simplifies maintenance. This should unbreak the wasm enscripten builds that currently fail due to linking in --whole-archive mode and the HACL* library appearing twice. Long unnoticed error fixed: `_sha512.SHA384Type` was doubly assigned and was actually SHA512Type. Nobody depends on those internal names.
gpshead · Feb 15, 2023 · fea4cdf · fea4cdf
1 parent d777790
commit fea4cdf
Show file tree

Hide file tree

Showing 17 changed files with 1,303 additions and 1,497 deletions.
diff --git a/Lib/hashlib.py b/Lib/hashlib.py
@@ -92,13 +92,13 @@ def __get_builtin_constructor(name):
             import _md5
             cache['MD5'] = cache['md5'] = _md5.md5
         elif name in {'SHA256', 'sha256', 'SHA224', 'sha224'}:
-            import _sha256
-            cache['SHA224'] = cache['sha224'] = _sha256.sha224
-            cache['SHA256'] = cache['sha256'] = _sha256.sha256
+            import _sha2
+            cache['SHA224'] = cache['sha224'] = _sha2.sha224
+            cache['SHA256'] = cache['sha256'] = _sha2.sha256
         elif name in {'SHA512', 'sha512', 'SHA384', 'sha384'}:
-            import _sha512
-            cache['SHA384'] = cache['sha384'] = _sha512.sha384
-            cache['SHA512'] = cache['sha512'] = _sha512.sha512
+            import _sha2
+            cache['SHA384'] = cache['sha384'] = _sha2.sha384
+            cache['SHA512'] = cache['sha512'] = _sha2.sha512
         elif name in {'blake2b', 'blake2s'}:
             import _blake2
             cache['blake2b'] = _blake2.blake2b

diff --git a/Lib/test/test_hashlib.py b/Lib/test/test_hashlib.py
@@ -172,12 +172,10 @@ def add_builtin_constructor(name):
         _sha1 = self._conditional_import_module('_sha1')
         if _sha1:
             add_builtin_constructor('sha1')
-        _sha256 = self._conditional_import_module('_sha256')
-        if _sha256:
+        _sha2 = self._conditional_import_module('_sha2')
+        if _sha2:
             add_builtin_constructor('sha224')
             add_builtin_constructor('sha256')
-        _sha512 = self._conditional_import_module('_sha512')
-        if _sha512:
             add_builtin_constructor('sha384')
             add_builtin_constructor('sha512')
         if _blake2:
@@ -371,19 +369,20 @@ def check(self, name, data, hexdigest, shake=False, **kwargs):
         # 2 is for hashlib.name(...) and hashlib.new(name, ...)
         self.assertGreaterEqual(len(constructors), 2)
         for hash_object_constructor in constructors:
-            m = hash_object_constructor(data, **kwargs)
-            computed = m.hexdigest() if not shake else m.hexdigest(length)
-            self.assertEqual(
-                    computed, hexdigest,
-                    "Hash algorithm %s constructed using %s returned hexdigest"
-                    " %r for %d byte input data that should have hashed to %r."
-                    % (name, hash_object_constructor,
-                       computed, len(data), hexdigest))
-            computed = m.digest() if not shake else m.digest(length)
-            digest = bytes.fromhex(hexdigest)
-            self.assertEqual(computed, digest)
-            if not shake:
-                self.assertEqual(len(digest), m.digest_size)
+            with self.subTest(implementation=hash_object_constructor):
+                m = hash_object_constructor(data, **kwargs)
+                computed = m.hexdigest() if not shake else m.hexdigest(length)
+                self.assertEqual(
+                        computed, hexdigest,
+                        "Hash algorithm %s constructed using %s returned hexdigest"
+                        " %r for %d byte input data that should have hashed to %r."
+                        % (name, hash_object_constructor,
+                           computed, len(data), hexdigest))
+                computed = m.digest() if not shake else m.digest(length)
+                digest = bytes.fromhex(hexdigest)
+                self.assertEqual(computed, digest)
+                if not shake:
+                    self.assertEqual(len(digest), m.digest_size)
 
         if not shake and kwargs.get("key") is None:
             # skip shake and blake2 extended parameter tests
@@ -404,14 +403,15 @@ def check_file_digest(self, name, data, hexdigest):
 
         try:
             for digest in digests:
-                buf = io.BytesIO(data)
-                buf.seek(0)
-                self.assertEqual(
-                    hashlib.file_digest(buf, digest).hexdigest(), hexdigest
-                )
-                with open(os_helper.TESTFN, "rb") as f:
-                    digestobj = hashlib.file_digest(f, digest)
-                self.assertEqual(digestobj.hexdigest(), hexdigest)
+                with self.subTest(msg="check_file_digest", implementation=digest):
+                    buf = io.BytesIO(data)
+                    buf.seek(0)
+                    self.assertEqual(
+                        hashlib.file_digest(buf, digest).hexdigest(), hexdigest
+                    )
+                    with open(os_helper.TESTFN, "rb") as f:
+                        digestobj = hashlib.file_digest(f, digest)
+                    self.assertEqual(digestobj.hexdigest(), hexdigest)
         finally:
             os.unlink(os_helper.TESTFN)
 

diff --git a/Makefile.pre.in b/Makefile.pre.in
@@ -2635,9 +2635,8 @@ MODULE__HASHLIB_DEPS=$(srcdir)/Modules/hashlib.h
 MODULE__IO_DEPS=$(srcdir)/Modules/_io/_iomodule.h
 MODULE__MD5_DEPS=$(srcdir)/Modules/hashlib.h
 MODULE__SHA1_DEPS=$(srcdir)/Modules/hashlib.h
-MODULE__SHA256_DEPS=$(srcdir)/Modules/hashlib.h $(LIBHACL_HEADERS) $(LIBHACL_A)
+MODULE__SHA2_DEPS=$(srcdir)/Modules/hashlib.h $(LIBHACL_HEADERS) $(LIBHACL_A)
 MODULE__SHA3_DEPS=$(srcdir)/Modules/_sha3/sha3.c $(srcdir)/Modules/_sha3/sha3.h $(srcdir)/Modules/hashlib.h
-MODULE__SHA512_DEPS=$(srcdir)/Modules/hashlib.h $(LIBHACL_HEADERS) $(LIBHACL_A)
 MODULE__SOCKET_DEPS=$(srcdir)/Modules/socketmodule.h $(srcdir)/Modules/addrinfo.h $(srcdir)/Modules/getaddrinfo.c $(srcdir)/Modules/getnameinfo.c
 MODULE__SSL_DEPS=$(srcdir)/Modules/_ssl.h $(srcdir)/Modules/_ssl/cert.c $(srcdir)/Modules/_ssl/debughelpers.c $(srcdir)/Modules/_ssl/misc.c $(srcdir)/Modules/_ssl_data.h $(srcdir)/Modules/_ssl_data_111.h $(srcdir)/Modules/_ssl_data_300.h $(srcdir)/Modules/socketmodule.h
 MODULE__TESTCAPI_DEPS=$(srcdir)/Modules/_testcapi/testcapi_long.h $(srcdir)/Modules/_testcapi/parts.h

diff --git a/Misc/NEWS.d/next/Library/2023-02-15-01-54-06.gh-issue-99108.rjTSic.rst b/Misc/NEWS.d/next/Library/2023-02-15-01-54-06.gh-issue-99108.rjTSic.rst
@@ -0,0 +1,3 @@
+The built-in extension modules for :mod:`hashlib` SHA2 algorithms, used when
+OpenSSL does provide them, now live in a single internal ``_sha2`` module
+instead of separate ``_sha256`` and ``_sha512`` modules.
diff --git a/Modules/Setup b/Modules/Setup
@@ -165,8 +165,7 @@ PYTHONPATH=$(COREPYTHONPATH)
 #_blake2 _blake2/blake2module.c _blake2/blake2b_impl.c _blake2/blake2s_impl.c
 #_md5 md5module.c
 #_sha1 sha1module.c
-#_sha256 sha256module.c
-#_sha512 sha512module.c
+#_sha2 sha2module.c -I$(srcdir)/Modules/_hacl/include Modules/_hacl/libHacl_Streaming_SHA2.a
 #_sha3 _sha3/sha3module.c
 
 # text encodings and unicode

diff --git a/Modules/Setup.stdlib.in b/Modules/Setup.stdlib.in
@@ -79,8 +79,7 @@
 # hashing builtins, can be disabled with --without-builtin-hashlib-hashes
 @MODULE__MD5_TRUE@_md5 md5module.c
 @MODULE__SHA1_TRUE@_sha1 sha1module.c
-@MODULE__SHA256_TRUE@_sha256 sha256module.c -I$(srcdir)/Modules/_hacl/include Modules/_hacl/libHacl_Streaming_SHA2.a
-@MODULE__SHA512_TRUE@_sha512 sha512module.c -I$(srcdir)/Modules/_hacl/include Modules/_hacl/libHacl_Streaming_SHA2.a
+@MODULE__SHA2_TRUE@_sha2 sha2module.c -I$(srcdir)/Modules/_hacl/include Modules/_hacl/libHacl_Streaming_SHA2.a
 @MODULE__SHA3_TRUE@_sha3 _sha3/sha3module.c
 @MODULE__BLAKE2_TRUE@_blake2 _blake2/blake2module.c _blake2/blake2b_impl.c _blake2/blake2s_impl.c
 

diff --git a/Modules/clinic/sha256module.c.h b/Modules/clinic/sha256module.c.h