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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 2 months ago by greg
Modified:
6 years, 1 month ago
Reviewers:
CC:
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
Visibility:
Public.

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

Messages

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

http://bugs.python.org/review/6721/diff/2612/6327
File Lib/test/lock_tests.py (right):

http://bugs.python.org/review/6721/diff/2612/6327#newcode305
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.

http://bugs.python.org/review/6721/diff/2612/6331
File Python/thread_pthread.h (right):

http://bugs.python.org/review/6721/diff/2612/6331#newcode183
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
pointer).

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
issue.

http://bugs.python.org/review/6721/diff/2612/6331#newcode400
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?

http://bugs.python.org/review/6721/diff/2612/6331#newcode451
Python/thread_pthread.h:451: pthread_lock *thelock = (pthread_lock *)lock;
same comment as above here.

http://bugs.python.org/review/6721/diff/2612/6331#newcode630
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