Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

#6721: Locks in python standard library should be sanitized on fork

Can't Edit
Can't Publish+Mail
Start Review
7 years, 2 months ago by greg
6 years, 1 month ago
gregory.p.smith, Vinay Sajip, jcea, nirs, AntoinePitrou, haypo, nirai, forest_alittletooquiet.net, ionelmc, bobbyi_gmail.com, Charles-Fran├žois Natali, giovannibajo_gmail.com, sdaoden_googlemail.com, tshepang, sbt, pybug.20.lesha_xoxy.net, A. Jesse Jiryu Davis, dan.oreilly, davin, lemuix_gmail.com, Winterflower, Birne94, ochedru_gmail.com

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Total comments: 5

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Include/pythread.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
Lib/test/lock_tests.py View 1 2 3 4 5 2 chunks +72 lines, -0 lines 0 comments Download
Lib/test/test_threading.py View 1 2 3 4 5 2 chunks +11 lines, -1 line 0 comments Download
Modules/_threadmodule.c View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
Modules/signalmodule.c View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
Python/thread.c View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
Python/thread_pthread.h View 1 2 3 4 5 13 chunks +122 lines, -12 lines 0 comments Download


Total messages: 1
6 years, 2 months ago #1
reinit_locks.diff is "Patch Set 3", i'm adding comments for that.

File Lib/test/lock_tests.py (right):

Lib/test/lock_tests.py:305: os._exit(0)
put os._exit(0) in a finally: with all of the above pid == 0 code within the
try: in all of these tests.  just to be 100% safe.

File Python/thread_pthread.h (right):

Python/thread_pthread.h:183: list_remove(pthread_lock **head, pthread_lock *e)
We need an internal lock to protect the locks_head list.  We shouldn't depend on
the GIL to protect it for us.

If the fork() is done by a C extension module (or code embedding a Python
interpreter) which then does its own PyOS_AfterFork() API call within the child
process (thus calling PyThread_ReinitLocks), the fork() can happen while the GIL
is held by a Python thread and was in the middle of modifying the linked list so
it could be in an inconsistent state (a partially filled in next or head

This can be deferred to a future change.  As is, this is already better than
what we have today.

Offering protection for extension or embedding code that fork()s from its own
non-Python threads while the python interpreter is running is worthy of its own

Python/thread_pthread.h:400: pthread_lock *thelock = (pthread_lock *)lock;
any reason for this not to be

sem_t *thelock = ((pthread_lock *)lock)->sem

so that the changes in this function below are not needed?

Python/thread_pthread.h:451: pthread_lock *thelock = (pthread_lock *)lock;
same comment as above here.

Python/thread_pthread.h:630: pthread_lock *l;
don't use single letter variable names.  in many fonts this looks like 1 or i
rather than l.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7