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

Kirill Smelkov added the comment: Ok, I did https://github.com/python/cpython/pull/16047. -- ___ Python tracker ___ ___

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

Change by Kirill Smelkov : -- keywords: +patch pull_requests: +15669 stage: -> patch review pull_request: https://github.com/python/cpython/pull/16047 ___ Python tracker ___

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

Armin Rigo added the comment: I agree with your analysis. I guess you (or someone) needs to write an explicit pull request, even if it just contains 187aa545165d cherry-picked. (I'm not a core dev any more nowadays) -- ___ Python tracker

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

Kirill Smelkov added the comment: I agree it seems like a design mistake. Not only it leads to suboptimal implementations, but what is more important, it throws misuse risks onto the user. -- ___ Python tracker

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

Kristján Valur Jónsson added the comment: imho posix made a mistake in allowing signal/broadcast outside the mutex. Otherwise an implementation could rely on the mutex for internal state manipulation. I have my own fast condition variable lib implemented using semaphores and it is simple

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

Kirill Smelkov added the comment: And it is indeed better to always do pthread_cond_signal() from under mutex. Many pthread libraries delay the signalling to associated mutex unlock, so there should be no performance penalty here and the correctness is much more easier to reason about.

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

Kirill Smelkov 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

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

Kristján Valur Jónsson added the comment: Interesting. Yet another reason to always do condition signalling with the lock held, such as is good practice to avoid race conditions. That's the whole point of condition variables. -- ___ Python

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

Armin Rigo added the comment: I may be wrong, but I believe that the bug requires using the C API (not just pure Python code). This is because Python-level lock objects have their own lifetime, and should never be freed while another thread is in PyThread_release_lock() with them.

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

Change by Guido van Rossum : -- assignee: -> benjamin.peterson nosy: -gvanrossum title: Race in PyThread_release_lock - can lead to MEMORY CORRUPTION and DEADLOCK -> Race in PyThread_release_lock - can lead to memory corruption and deadlock ___

[issue38106] Race in PyThread_release_lock - can lead to MEMORY CORRUPTION and DEADLOCK

New submission from Kirill Smelkov : Hello up there. I believe I've discovered a race in PyThread_release_lock on Python2.7 that, on systems where POSIX semaphores are not available and Python locks are implemented with mutexes and condition variables, can lead to MEMORY CORRUPTION and