unconditionally lock when performing HASH updates

Author: picnixz

Lib/test/support/hashlib_helper.py

@@ -308,22 +308,3 @@ def sha3_384(self):
     @property
     def sha3_512(self):
         return self._find_constructor_in("_sha3","sha3_512")
-
-
-def find_gil_minsize(modules_names, default=2048):
-    """Get the largest GIL_MINSIZE value for the given cryptographic modules.
-
-    The valid module names are the following:
-
-    - _hashlib
-    - _md5, _sha1, _sha2, _sha3, _blake2
-    - _hmac
-    """
-    sizes = []
-    for module_name in modules_names:
-        try:
-            module = importlib.import_module(module_name)
-        except ImportError:
-            continue
-        sizes.append(getattr(module, '_GIL_MINSIZE', default))
-    return max(sizes, default=default)

Lib/test/test_hashlib.py

@@ -409,7 +409,7 @@ def test_large_update(self):
         aas = b'a' * 128
         bees = b'b' * 127
         cees = b'c' * 126
-        dees = b'd' * 2048 #  HASHLIB_GIL_MINSIZE
+        dees = b'd' * 2048
 
         for cons in self.hash_constructors:
             m1 = cons(usedforsecurity=False)
@@ -990,40 +990,6 @@ def test_case_shake256_vector(self):
         for msg, md in read_vectors('shake_256'):
             self.check('shake_256', msg, md, True)
 
-    def test_gil(self):
-        # Check things work fine with an input larger than the size required
-        # for multithreaded operation. Currently, all cryptographic modules
-        # have the same constant value (2048) but in the future it might not
-        # be the case.
-        mods = ['_md5', '_sha1', '_sha2', '_sha3', '_blake2', '_hashlib']
-        gil_minsize = hashlib_helper.find_gil_minsize(mods)
-        for cons in self.hash_constructors:
-            # constructors belong to one of the above modules
-            m = cons(usedforsecurity=False)
-            m.update(b'1')
-            m.update(b'#' * gil_minsize)
-            m.update(b'1')
-
-            m = cons(b'x' * gil_minsize, usedforsecurity=False)
-            m.update(b'1')
-
-    def test_sha256_gil(self):
-        gil_minsize = hashlib_helper.find_gil_minsize(['_sha2', '_hashlib'])
-        m = hashlib.sha256()
-        m.update(b'1')
-        m.update(b'#' * gil_minsize)
-        m.update(b'1')
-        self.assertEqual(
-            m.hexdigest(),
-            '1cfceca95989f51f658e3f3ffe7f1cd43726c9e088c13ee10b46f57cef135b94'
-        )
-
-        m = hashlib.sha256(b'1' + b'#' * gil_minsize + b'1')
-        self.assertEqual(
-            m.hexdigest(),
-            '1cfceca95989f51f658e3f3ffe7f1cd43726c9e088c13ee10b46f57cef135b94'
-        )
-
     @threading_helper.reap_threads
     @threading_helper.requires_working_threading()
     def test_threaded_hashing(self):

Lib/test/test_hmac.py

@@ -1133,11 +1133,6 @@ def HMAC(self, key, msg=None):
         """Create a HMAC object."""
         raise NotImplementedError
 
-    @property
-    def gil_minsize(self):
-        """Get the maximal input length for the GIL to be held."""
-        raise NotImplementedError
-
     def check_update(self, key, chunks):
         chunks = list(chunks)
         msg = b''.join(chunks)
@@ -1155,13 +1150,6 @@ def test_update(self):
         with self.subTest(key=key, msg=msg):
             self.check_update(key, [msg])
 
-    def test_update_large(self):
-        gil_minsize = self.gil_minsize
-        key = random.randbytes(16)
-        top = random.randbytes(gil_minsize + 1)
-        bot = random.randbytes(gil_minsize + 1)
-        self.check_update(key, [top, bot])
-
     def test_update_exceptions(self):
         h = self.HMAC(b"key")
         for msg in ['invalid msg', 123, (), []]:
@@ -1175,21 +1163,13 @@ class PyUpdateTestCase(PyModuleMixin, UpdateTestCaseMixin, unittest.TestCase):
     def HMAC(self, key, msg=None):
         return self.hmac.HMAC(key, msg, digestmod='sha256')
 
-    @property
-    def gil_minsize(self):
-        return sha2._GIL_MINSIZE
-
 
 @hashlib_helper.requires_openssl_hashdigest('sha256')
 class OpenSSLUpdateTestCase(UpdateTestCaseMixin, unittest.TestCase):
 
     def HMAC(self, key, msg=None):
         return _hashlib.hmac_new(key, msg, digestmod='sha256')
 
-    @property
-    def gil_minsize(self):
-        return _hashlib._GIL_MINSIZE
-
 
 class BuiltinUpdateTestCase(BuiltinModuleMixin,
                             UpdateTestCaseMixin, unittest.TestCase):
@@ -1199,10 +1179,6 @@ def HMAC(self, key, msg=None):
         # are still built, making it possible to use SHA-2 hashes.
         return self.hmac.new(key, msg, digestmod='sha256')
 
-    @property
-    def gil_minsize(self):
-        return self.hmac._GIL_MINSIZE
-
 
 class CopyBaseTestCase:
 

Modules/_hashopenssl.c

@@ -611,9 +611,9 @@ static int
 _hashlib_HASH_copy_locked(HASHobject *self, EVP_MD_CTX *new_ctx_p)
 {
     int result;
-    ENTER_HASHLIB(self);
+    HASHLIB_ACQUIRE_LOCK(self);
     result = EVP_MD_CTX_copy(new_ctx_p, self->ctx);
-    LEAVE_HASHLIB(self);
+    HASHLIB_RELEASE_LOCK(self);
     return result;
 }
 
@@ -733,29 +733,16 @@ static PyObject *
 _hashlib_HASH_update_impl(HASHobject *self, PyObject *obj)
 /*[clinic end generated code: output=62ad989754946b86 input=aa1ce20e3f92ceb6]*/
 {
-    int result;
+    int rc;
     Py_buffer view;
-
     GET_BUFFER_VIEW_OR_ERROUT(obj, &view);
-
-    if (!self->use_mutex && view.len >= HASHLIB_GIL_MINSIZE) {
-        self->use_mutex = true;
-    }
-    if (self->use_mutex) {
-        Py_BEGIN_ALLOW_THREADS
-        PyMutex_Lock(&self->mutex);
-        result = _hashlib_HASH_hash(self, view.buf, view.len);
-        PyMutex_Unlock(&self->mutex);
-        Py_END_ALLOW_THREADS
-    } else {
-        result = _hashlib_HASH_hash(self, view.buf, view.len);
-    }
-
+    Py_BEGIN_ALLOW_THREADS
+        HASHLIB_ACQUIRE_LOCK(self);
+        rc = _hashlib_HASH_hash(self, view.buf, view.len);
+        HASHLIB_RELEASE_LOCK(self);
+    Py_END_ALLOW_THREADS
     PyBuffer_Release(&view);
-
-    if (result == -1)
-        return NULL;
-    Py_RETURN_NONE;
+    return rc < 0 ? NULL : Py_None;
 }
 
 static PyMethodDef HASH_methods[] = {
@@ -1060,15 +1047,11 @@ _hashlib_HASH(PyObject *module, const char *digestname, PyObject *data_obj,
     }
 
     if (view.buf && view.len) {
-        if (view.len >= HASHLIB_GIL_MINSIZE) {
-            /* We do not initialize self->lock here as this is the constructor
-             * where it is not yet possible to have concurrent access. */
-            Py_BEGIN_ALLOW_THREADS
-            result = _hashlib_HASH_hash(self, view.buf, view.len);
-            Py_END_ALLOW_THREADS
-        } else {
+        /* Do not use self->mutex here as this is the constructor
+         * where it is not yet possible to have concurrent access. */
+        Py_BEGIN_ALLOW_THREADS
             result = _hashlib_HASH_hash(self, view.buf, view.len);
-        }
+        Py_END_ALLOW_THREADS
         if (result == -1) {
             assert(PyErr_Occurred());
             Py_CLEAR(self);
@@ -1701,7 +1684,7 @@ _hashlib_hmac_new_impl(PyObject *module, Py_buffer *key, PyObject *msg_obj,
     HASHLIB_INIT_MUTEX(self);
 
     if ((msg_obj != NULL) && (msg_obj != Py_None)) {
-        if (!_hmac_update(self, msg_obj)) {
+        if (_hmac_update(self, msg_obj) < 0) {
             goto error;
         }
     }
@@ -1718,9 +1701,9 @@ static int
 locked_HMAC_CTX_copy(HMAC_CTX *new_ctx_p, HMACobject *self)
 {
     int result;
-    ENTER_HASHLIB(self);
+    HASHLIB_ACQUIRE_LOCK(self);
     result = HMAC_CTX_copy(new_ctx_p, self->ctx);
-    LEAVE_HASHLIB(self);
+    HASHLIB_RELEASE_LOCK(self);
     return result;
 }
 
@@ -1743,35 +1726,26 @@ _hashlib_hmac_digest_size(HMACobject *self)
 static int
 _hmac_update(HMACobject *self, PyObject *obj)
 {
-    int r;
+    int r = 1;
     Py_buffer view = {0};
 
-    GET_BUFFER_VIEW_OR_ERROR(obj, &view, return 0);
-
-    if (!self->use_mutex && view.len >= HASHLIB_GIL_MINSIZE) {
-        self->use_mutex = true;
-    }
-    if (self->use_mutex) {
+    GET_BUFFER_VIEW_OR_ERROR(obj, &view, return -1);
+    if (view.len > 0) {
         Py_BEGIN_ALLOW_THREADS
-        PyMutex_Lock(&self->mutex);
-        r = HMAC_Update(self->ctx,
-                        (const unsigned char *)view.buf,
-                        (size_t)view.len);
-        PyMutex_Unlock(&self->mutex);
+            HASHLIB_ACQUIRE_LOCK(self);
+            r = HMAC_Update(self->ctx,
+                            (const unsigned char *)view.buf,
+                            (size_t)view.len);
+            HASHLIB_RELEASE_LOCK(self);
         Py_END_ALLOW_THREADS
-    } else {
-        r = HMAC_Update(self->ctx,
-                        (const unsigned char *)view.buf,
-                        (size_t)view.len);
     }
-
     PyBuffer_Release(&view);
 
     if (r == 0) {
         notify_ssl_error_occurred();
-        return 0;
+        return -1;
     }
-    return 1;
+    return 0;
 }
 
 /*[clinic input]
@@ -1845,7 +1819,7 @@ static PyObject *
 _hashlib_HMAC_update_impl(HMACobject *self, PyObject *msg)
 /*[clinic end generated code: output=f31f0ace8c625b00 input=1829173bb3cfd4e6]*/
 {
-    if (!_hmac_update(self, msg)) {
+    if (_hmac_update(self, msg) < 0) {
         return NULL;
     }
     Py_RETURN_NONE;
@@ -2412,17 +2386,6 @@ hashlib_exception(PyObject *module)
     return 0;
 }
 
-static int
-hashlib_constants(PyObject *module)
-{
-    if (PyModule_AddIntConstant(module, "_GIL_MINSIZE",
-                                HASHLIB_GIL_MINSIZE) < 0)
-    {
-        return -1;
-    }
-    return 0;
-}
-
 static PyModuleDef_Slot hashlib_slots[] = {
     {Py_mod_exec, hashlib_init_hashtable},
     {Py_mod_exec, hashlib_init_HASH_type},
@@ -2431,7 +2394,6 @@ static PyModuleDef_Slot hashlib_slots[] = {
     {Py_mod_exec, hashlib_md_meth_names},
     {Py_mod_exec, hashlib_init_constructors},
     {Py_mod_exec, hashlib_exception},
-    {Py_mod_exec, hashlib_constants},
     {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED},
     {Py_mod_gil, Py_MOD_GIL_NOT_USED},
     {0, NULL}