[issue38106] Race in PyThread_release_lock - can lead to memory corruption and deadlock

[Kirill Smelkov]

Kirill Smelkov <kirr at nexedi.com> added the comment:

Thanks for feedback. Yes, since for Python-level lock, PyThread_release_lock() is called with GIL held:

https://github.com/python/cpython/blob/v2.7.16-129-g58d61efd4cd/Modules/threadmodule.c#L69-L82

the GIL effectively serves as the synchronization device in between T2
releasing the lock, and T1 proceeding after second sema.acquire() when it gets to
execute python-level code with `del sema`.

However

a) there is no sign that this aspect - that release must be called under GIL -
   is being explicitly relied upon by PyThread_release_lock() code, and

b) e.g. _testcapimodule.c already has a test which calls
   PyThread_release_lock() with GIL released:

   https://github.com/python/cpython/blob/v2.7.16-129-g58d61efd4cd/Modules/_testcapimodule.c#L1972-L2053
   https://github.com/python/cpython/blob/v2.7.16-129-g58d61efd4cd/Modules/_testcapimodule.c#L1998-L2002

Thus, I believe, there is a bug in PyThread_release_lock() and we were just
lucky not to hit it due to GIL and Python-level usage.

For the reference, I indeed started to observe the problem when I moved locks
and other code that implement channels in Pygolang from Python to C level:

https://lab.nexedi.com/kirr/pygolang/commit/69db91bf
https://lab.nexedi.com/kirr/pygolang/commit/3b241983?expand_all_diffs=1

----------

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue38106>
_______________________________________