Blake2b/s implementations have minor GIL issues

[gwk]

BPO	36519
Nosy	@tiran, @gwk

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tiran'
closed_at = None
created_at = <Date 2019-04-03.15:04:35.230>
labels = ['extension-modules', '3.8', 'type-bug', '3.7']
title = 'Blake2b/s implementations have minor GIL issues'
updated_at = <Date 2019-04-03.15:20:57.965>
user = 'https://github.com/gwk'

bugs.python.org fields:

activity = <Date 2019-04-03.15:20:57.965>
actor = 'christian.heimes'
assignee = 'christian.heimes'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2019-04-03.15:04:35.230>
creator = 'gwk'
dependencies = []
files = []
hgrepos = []
issue_num = 36519
keywords = []
message_count = 2.0
messages = ['339394', '339396']
nosy_count = 2.0
nosy_names = ['christian.heimes', 'gwk']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36519'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[gwk]

I was browsing the Blake2b module implementation in master and noticed two subtle issues in blake2b_impl.c. There are two places where the GIL gets released; both of them appear flawed.

py_blake2b_new_impl, line 221. The ALLOW_THREADS block fails to acquire/release self->lock.

_blake2_blake2b_update, line 279. The lock is lazily allocated correctly on line 279. However the test on 282 that chooses to release the GIL or not fails to take into account the length test. This means that once a large block is fed to update, then every call to update will release the GIL, even if it is a single byte.

It should look something more like this:

bool should_allow_threads = (buf.len >= HASHLIB_GIL_MINSIZE);
if (should_allow_threads && self->lock == NULL)
  self->lock = PyThread_allocate_lock();
if (should_allow_threads && self->lock != NULL) { ... }
else { ... }

This respects the size criterion, and also protects against the case where the lock allocation fails.

[tiran]

Thanks, I'll have a look.