From d1a214a2fa22135e40f69e90b5c429e80064dca7 Mon Sep 17 00:00:00 2001 From: Armin Rigo Date: Wed, 12 Apr 2017 11:36:43 +0200 Subject: [PATCH 1/4] Issue #29694: race condition in pathlib mkdir with flags parents=True --- Lib/pathlib.py | 4 ++-- Lib/test/test_pathlib.py | 31 +++++++++++++++++++++++++++++++ Misc/NEWS | 3 +++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index fc7ce5eb2ab908..19142295c9b79f 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -1217,8 +1217,8 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False): except FileNotFoundError: if not parents or self.parent == self: raise - self.parent.mkdir(parents=True) - self._accessor.mkdir(self, mode) + self.parent.mkdir(parents=True, exist_ok=True) + self.mkdir(mode, parents=False, exist_ok=exist_ok) except OSError: # Cannot rely on checking for EEXIST, since the operating system # could give priority to other errors like EACCES or EROFS diff --git a/Lib/test/test_pathlib.py b/Lib/test/test_pathlib.py index 46a705e1cff5c2..1fbdff60ef846b 100644 --- a/Lib/test/test_pathlib.py +++ b/Lib/test/test_pathlib.py @@ -8,6 +8,7 @@ import stat import tempfile import unittest +from unittest import mock from test import support android_not_root = support.android_not_root @@ -1801,6 +1802,36 @@ def test_mkdir_no_parents_file(self): p.mkdir(exist_ok=True) self.assertEqual(cm.exception.errno, errno.EEXIST) + def test_mkdir_concurrent_parent_creation(self): + for pattern_num in range(32): + p = self.cls(BASE, 'dirCPC%d' % pattern_num) + self.assertFalse(p.exists()) + + def my_mkdir(path, mode=0o777): + path = str(path) + # Emulate another process that would create the directory + # just before we try to create it ourselves. We do it + # in all possible pattern combinations, assuming that this + # function is called at most 5 times (dirCPC/dir1/dir2, + # dirCPC/dir1, dirCPC, dirCPC/dir1, cirCPC/dir1/dir2). + if pattern.pop(): + os.mkdir(path, mode) # from another process + concurrently_created.add(path) + os.mkdir(path, mode) # our real call + org_mkdir = pathlib._normal_accessor.mkdir + + pattern = [bool(pattern_num & (1 << n)) for n in range(5)] + concurrently_created = set() + p12 = p / 'dir1' / 'dir2' + try: + with mock.patch("pathlib._normal_accessor.mkdir", my_mkdir): + p12.mkdir(parents=True, exist_ok=False) + got_exception = False + except FileExistsError: + got_exception = True + self.assertEqual(str(p12) in concurrently_created, got_exception) + self.assertTrue(p.exists()) + @support.skip_unless_symlink def test_symlink_to(self): P = self.cls(BASE) diff --git a/Misc/NEWS b/Misc/NEWS index d8b1ccb43a2059..899a95878dac3f 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -306,6 +306,9 @@ Extension Modules Library ------- + +- bpo-29694: race condition in pathlib mkdir with flags parents=True + - bpo-29692: Fixed arbitrary unchaining of RuntimeError exceptions in contextlib.contextmanager. Patch by Siddharth Velankar. From f77b3db6c8273a17f67b434ad7d6eb75c6a97d32 Mon Sep 17 00:00:00 2001 From: Armin Rigo Date: Wed, 12 Apr 2017 16:12:56 +0200 Subject: [PATCH 2/4] Fixes from feedback (thanks Serhiy) --- Lib/test/test_pathlib.py | 7 +++---- Misc/NEWS | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_pathlib.py b/Lib/test/test_pathlib.py index 1fbdff60ef846b..23c69dfa3c4734 100644 --- a/Lib/test/test_pathlib.py +++ b/Lib/test/test_pathlib.py @@ -1818,7 +1818,6 @@ def my_mkdir(path, mode=0o777): os.mkdir(path, mode) # from another process concurrently_created.add(path) os.mkdir(path, mode) # our real call - org_mkdir = pathlib._normal_accessor.mkdir pattern = [bool(pattern_num & (1 << n)) for n in range(5)] concurrently_created = set() @@ -1826,10 +1825,10 @@ def my_mkdir(path, mode=0o777): try: with mock.patch("pathlib._normal_accessor.mkdir", my_mkdir): p12.mkdir(parents=True, exist_ok=False) - got_exception = False except FileExistsError: - got_exception = True - self.assertEqual(str(p12) in concurrently_created, got_exception) + self.assertIn(str(p12), concurrently_created) + else: + self.assertNotIn(str(p12), concurrently_created) self.assertTrue(p.exists()) @support.skip_unless_symlink diff --git a/Misc/NEWS b/Misc/NEWS index 899a95878dac3f..e736fde75ffec1 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -307,7 +307,8 @@ Extension Modules Library ------- -- bpo-29694: race condition in pathlib mkdir with flags parents=True +- bpo-29694: Fixed race condition in pathlib mkdir with flags + parents=True. - bpo-29692: Fixed arbitrary unchaining of RuntimeError exceptions in contextlib.contextmanager. From 29af33bec80402888e1937908947765a8273aa3c Mon Sep 17 00:00:00 2001 From: Armin Rigo Date: Thu, 13 Apr 2017 08:00:58 +0200 Subject: [PATCH 3/4] typo --- Lib/test/test_pathlib.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_pathlib.py b/Lib/test/test_pathlib.py index 23c69dfa3c4734..21a6390e352a85 100644 --- a/Lib/test/test_pathlib.py +++ b/Lib/test/test_pathlib.py @@ -1813,7 +1813,7 @@ def my_mkdir(path, mode=0o777): # just before we try to create it ourselves. We do it # in all possible pattern combinations, assuming that this # function is called at most 5 times (dirCPC/dir1/dir2, - # dirCPC/dir1, dirCPC, dirCPC/dir1, cirCPC/dir1/dir2). + # dirCPC/dir1, dirCPC, dirCPC/dir1, dirCPC/dir1/dir2). if pattern.pop(): os.mkdir(path, mode) # from another process concurrently_created.add(path) From a3397be470ed473ca8eb9c420badd5e69747d227 Mon Sep 17 00:00:00 2001 From: Mariatta Date: Thu, 13 Apr 2017 10:35:09 -0700 Subject: [PATCH 4/4] Add `Patch By Armin Rigo` to Misc/NEWS --- Misc/NEWS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS b/Misc/NEWS index e736fde75ffec1..304ef3e3322148 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -308,7 +308,7 @@ Library ------- - bpo-29694: Fixed race condition in pathlib mkdir with flags - parents=True. + parents=True. Patch by Armin Rigo. - bpo-29692: Fixed arbitrary unchaining of RuntimeError exceptions in contextlib.contextmanager.