classification
Title: OSError 17 due to _multiprocessing/semaphore.c assuming a one-to-one Pid -> process mapping.
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.3, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: davin Nosy List: Arfrever, Paul Hobbs, benjamin.peterson, davin, djc, floppymaster, jnoller, neologix, python-dev, ryan.petrello, sbt, vapier, vstinner
Priority: normal Keywords: needs review, patch

Created on 2015-05-27 18:22 by Paul Hobbs, last changed 2018-11-01 05:41 by benjamin.peterson.

Files
File name Uploaded Description Edit
test-issue24303.py vapier, 2015-06-02 07:38
mp_sem_race.diff neologix, 2015-06-11 06:11 review
Pull Requests
URL Status Linked Edit
PR 10279 closed benjamin.peterson, 2018-11-01 04:47
Messages (17)
msg244211 - (view) Author: Paul Hobbs (Paul Hobbs) Date: 2015-05-27 18:22
Using pid namespacing it is possible to have multiple processes with the same pid.  "semlock_new" creates a semaphore file with the template "/dev/shm/mp{pid}-{counter}".  This can conflict if the same semaphore file already exists due to another Python process have the same pid.

This bug has been fixed in Python3: https://bugs.python.org/issue8713.  However, that patch is very large (40 files, ~4.4k changed lines) and only incidentally fixes this bug while introducing a large backwards-incompatible refactoring and feature addition.

The following small patch to just _multiprocessing/semaphore.c fixes the problem by using the system clock and retrying to avoid conflicts:

--- a/Modules/_multiprocessing/semaphore.c
+++ b/Modules/_multiprocessing/semaphore.c
@@ -7,6 +7,7 @@
  */
 
 #include "multiprocessing.h"
+#include <time.h>
 
 enum { RECURSIVE_MUTEX, SEMAPHORE };
 
@@ -419,7 +420,7 @@ semlock_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
 {
     char buffer[256];
     SEM_HANDLE handle = SEM_FAILED;
-    int kind, maxvalue, value;
+    int kind, maxvalue, value, try;
     PyObject *result;
     static char *kwlist[] = {"kind", "value", "maxvalue", NULL};
     static int counter = 0;
@@ -433,10 +434,24 @@ semlock_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
         return NULL;
     }
 
-    PyOS_snprintf(buffer, sizeof(buffer), "/mp%ld-%d", (long)getpid(), counter++);
+    /* With pid namespaces, we may have multiple processes with the same pid.
+     * Instead of relying on the pid to be unique, we use the microseconds time
+     * to attempt to a unique filename. */
+    for (try = 0; try < 100; ++try) {
+        struct timespec tv;
+        long arbitrary = clock_gettime(CLOCK_REALTIME, &tv) ? 0 : tv.tv_nsec;
+        PyOS_snprintf(buffer, sizeof(buffer), "/mp%ld-%d-%ld",
+                      (long)getpid(),
+                      counter++,
+                      arbitrary);
+        SEM_CLEAR_ERROR();
+        handle = SEM_CREATE(buffer, value, maxvalue);
+        if (handle != SEM_FAILED)
+            break;
+        else if (errno != EEXIST)
+            goto failure;
+    }
 
-    SEM_CLEAR_ERROR();
-    handle = SEM_CREATE(buffer, value, maxvalue);
     /* On Windows we should fail if GetLastError()==ERROR_ALREADY_EXISTS */
     if (handle == SEM_FAILED || SEM_GET_LAST_ERROR() != 0)
         goto failure;
msg244425 - (view) Author: Davin Potts (davin) * (Python committer) Date: 2015-05-29 22:26
At first blush it does appear there is potential for conflict because of how the semaphore filename template was implemented -- that's a cool find.  In practice, I wonder how often this has actually bitten anyone in the real world.  The Linux world's use of clone() (creating pid namespaces) is relatively new/young.  The BSD world's use of jails (bsd-style) take a bit of a different approach, advocate against the use of shared filesystems across jails where a similar conflict could arise, and have been around longer.

@Paul:  Out of curiosity, what inspired your discovery?

Agreed that backporting Richard's work from issue8713 does not appeal.

A few concerns:
* Retrying with a modified filename makes sense but basing it on a timestamp from the system clock is not particularly robust given that the cloned processes can be executing in sufficiently close lock step that they both get the same timestamp (given the granularity/precision of the clock functions).  Developers new to high performance computing often learn this the hard way when trying to use the system clock to create uniquely named files.
* Instead, what about using the system's available pseudo-random number generators?  Most are implemented to avoid just this problem where two or more processes/threads ask for a random/distinct value at nearly the same moment.
* What about simply using a counter (not the same static int counter but another) and incrementing it when the attempt to create the semaphore file fails?  This avoids a system function call and potentially simplifies things.  Would this be faster in the majority of cases?
msg244435 - (view) Author: Mike Frysinger (vapier) Date: 2015-05-30 02:18
we hit this problem daily in Chromium OS's build farm because we use pid namespaces heavily
msg244592 - (view) Author: Davin Potts (davin) * (Python committer) Date: 2015-06-01 15:22
Triggering it regularly in a build farm indeed sounds like genuine pain.

@Paul or @vapier: In tracking down this issue, did you already create a convenient way to repro the misbehavior that could be used in testing?  Any finalized patch that we make will need some form of test.
msg244657 - (view) Author: Mike Frysinger (vapier) Date: 2015-06-02 07:38
the original report on our side w/bunches of tracebacks: http://crbug.com/481223

with that traceback in hand, it's pretty trivial to write a reproduction :).  attached!

i couldn't figure out how to make it work w/out completely execing a new python instance.  i suspect the internals were communicating and thus defeating the race.

the unshare() might need some checks so that it skips on older kernels or ones with pidns support disabled.  but i don't have any such system anymore ;).

if all goes well, it should fail fairly quickly:
$ python3.3 ./test.py 
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib64/python3.3/multiprocessing/__init__.py", line 187, in Event
    return Event()
  File "/usr/lib64/python3.3/multiprocessing/synchronize.py", line 293, in __init__
    self._cond = Condition(Lock())
  File "/usr/lib64/python3.3/multiprocessing/synchronize.py", line 174, in __init__
    self._wait_semaphore = Semaphore(0)
  File "/usr/lib64/python3.3/multiprocessing/synchronize.py", line 84, in __init__
    SemLock.__init__(self, SEMAPHORE, value, SEM_VALUE_MAX)
  File "/usr/lib64/python3.3/multiprocessing/synchronize.py", line 48, in __init__
    sl = self._semlock = _multiprocessing.SemLock(kind, value, maxvalue)
FileExistsError: [Errno 17] File exists
failed

and doesn't take that long to pass:
$ time python3.4 ./test-issue24303.py 
passed!
real    0m1.715s
msg244658 - (view) Author: Mike Frysinger (vapier) Date: 2015-06-02 07:45
also, it looks like this bug is in python-3.3.  not sure what the status of that branch is, but maybe the backport from 3.4 would be easy ?
msg244797 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-06-04 04:22
I think it would be a better idea to partially backport the 2.7 logic that uses the random module.
msg245156 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2015-06-11 06:11
Here's a patch against 2.7 using _PyOS_URandom(): it should apply as-is to 3.3.
msg260058 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2016-02-10 23:10
Anyone opposed to me committing the patch I submitted?
It slves a real problem, and is fairly straight-forward (and conceptually more correct).
msg260061 - (view) Author: Mike Frysinger (vapier) Date: 2016-02-10 23:42
i don't feel strongly about either version
msg260185 - (view) Author: Davin Potts (davin) * (Python committer) Date: 2016-02-12 15:05
@neologix: I second your proposed patch -- looks like a winner to me.  Apologies for not following up earlier.
msg260208 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-12 22:39
New changeset d3662c088db8 by Charles-François Natali in branch '2.7':
Issue #24303: Fix random EEXIST upon multiprocessing semaphores creation with
https://hg.python.org/cpython/rev/d3662c088db8
msg328934 - (view) Author: Ryan Petrello (ryan.petrello) Date: 2018-10-30 15:59
Any chance this patch was every applied to Python3?  It looks to me like 3.6 has the old code:

https://github.com/python/cpython/blob/3.6/Modules/_multiprocessing/semaphore.c#L448
msg328944 - (view) Author: Mike Frysinger (vapier) Date: 2018-10-30 20:21
it does seem like the patch was never applied to any python 3 branch :/
msg329043 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-11-01 04:58
The retry logic is implemented at a different layer in Python 3: https://github.com/python/cpython/blob/a1c249c40517917d2e0971d55aea8d14a44b2cc8/Lib/multiprocessing/synchronize.py#L115-L117
msg329045 - (view) Author: Mike Frysinger (vapier) Date: 2018-11-01 05:40
that's highlighting the SemLock._make_name func which doesn't have retry logic in it.  did you mean to highlight SemLock.__init__ which has a retry loop that looks similar to the C code ?
https://github.com/python/cpython/blob/a1c249c40517917d2e0971d55aea8d14a44b2cc8/Lib/multiprocessing/synchronize.py#L55-L65
msg329046 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-11-01 05:41
Yes

On Wed, Oct 31, 2018, at 22:40, Mike Frysinger wrote:
>
> Mike Frysinger <vapier@users.sourceforge.net> added the comment:
>
> that's highlighting the SemLock._make_name func which doesn't
> have retry> logic in it.  did you mean to highlight SemLock.__init__ which has a
> retry loop that looks similar to the C code ?
> https://github.com/python/cpython/blob/a1c249c40517917d2e0971d55aea8d14a44b2cc8/Lib/multiprocessing/synchronize.py#L55-L65>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue24303>
> _______________________________________
History
Date User Action Args
2018-11-01 05:41:44benjamin.petersonsetmessages: + msg329046
2018-11-01 05:40:47vapiersetmessages: + msg329045
2018-11-01 04:58:38benjamin.petersonsetmessages: + msg329043
2018-11-01 04:47:21benjamin.petersonsetpull_requests: + pull_request9590
2018-10-30 20:21:37vapiersetmessages: + msg328944
2018-10-30 15:59:23ryan.petrellosetnosy: + ryan.petrello
messages: + msg328934
2016-02-12 22:39:34python-devsetnosy: + python-dev
messages: + msg260208
2016-02-12 15:05:46davinsetmessages: + msg260185
2016-02-10 23:42:35vapiersetmessages: + msg260061
2016-02-10 23:10:24neologixsetmessages: + msg260058
2015-06-13 13:18:19neologixsetkeywords: + needs review
nosy: + vstinner
2015-06-11 06:11:51neologixsetfiles: + mp_sem_race.diff
versions: + Python 3.3
nosy: + neologix

messages: + msg245156

keywords: + patch
2015-06-04 04:22:32benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg244797
2015-06-02 07:45:32vapiersetmessages: + msg244658
2015-06-02 07:38:11vapiersetfiles: + test-issue24303.py

messages: + msg244657
2015-06-01 15:22:03davinsetassignee: davin
type: enhancement -> behavior
messages: + msg244592
components: + Library (Lib)
2015-06-01 04:30:27yselivanovsetnosy: + jnoller
2015-05-30 02:18:00vapiersetmessages: + msg244435
2015-05-29 22:26:25davinsetmessages: + msg244425
2015-05-28 07:21:43djcsetnosy: + djc
2015-05-28 02:00:12floppymastersetnosy: + floppymaster
2015-05-27 19:36:19Arfreversetnosy: + Arfrever
2015-05-27 18:26:28vapiersetnosy: + vapier
2015-05-27 18:26:02ned.deilysetnosy: + davin, - devin
2015-05-27 18:24:39ned.deilysetnosy: + devin, sbt

stage: patch review
2015-05-27 18:22:59Paul Hobbscreate