From 4b7d59d23c460c2350480f2686941bc02c4f8848 Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sun, 13 Mar 2022 19:08:10 +0200 Subject: [PATCH 01/15] Gracefully handle expected errors returned by `FindFirstFileW` in `win32_xstat_impl` --- Modules/posixmodule.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 700cbd2617ad88..ac285d450b3f3c 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -1888,7 +1888,19 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result, /* Try reading the parent directory. */ if (!attributes_from_dir(path, &fileInfo, &tagInfo.ReparseTag)) { /* Cannot read the parent directory. */ - SetLastError(error); + DWORD dir_error = GetLastError(); + switch (GetLastError()) { + case ERROR_FILE_NOT_FOUND: + // TODO: Check why should I catch these? + case ERROR_PATH_NOT_FOUND: + case ERROR_NOT_READY: + case ERROR_BAD_NET_NAME: + break; + // restore the error from CreateFileW() + default: + SetLastError(error); + } + return -1; } if (fileInfo.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) { From 3c841f36a232aea2db4a4b9d2f25f8fbb2d118a7 Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sun, 13 Mar 2022 21:34:34 +0200 Subject: [PATCH 02/15] Add test for bugfix --- Lib/test/test_os.py | 47 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index ae8d930a95253a..8c990c4b4c1ea2 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2873,6 +2873,53 @@ def test_getfinalpathname_handles(self): self.assertEqual(0, handle_delta) + @support.requires_subprocess() + def test_stat_unlink_race(self): + # bpo-46785: the implementation of os.stat() falls back to reading + # the parent directory if CreateFileW() fails with a permission + # error. If reading the parent directory fails because the file or + # directory are subsequently unlinked, or because the volume or + # share are no longer available, then the original permission error + # should not be restored. + # TODO: Figure out why can't `fname` be simply `os_helper.TESTFN`? + fname = os.path.join(os.environ['TEMP'], os_helper.TESTFN + '_46785') + self.addCleanup(os_helper.unlink, fname) + # TODO: Think about putting this in an actual file? + command = '''if 1: + import os + import sys + fname = sys.argv[1] + while True: + try: + with open(fname, "w") as f: + pass + except OSError: + pass + try: + os.remove(fname) + except OSError: + pass + ''' + ignored_errors = ( + 2, # ERROR_FILE_NOT_FOUND + 3, # ERROR_PATH_NOT_FOUND + 21, # ERROR_NOT_READY + 67, # ERROR_BAD_NET_NAME + ) + deadline = time.time() + 5 + p = subprocess.Popen([sys.executable, '-c', command, fname]) + + try: + while time.time() < deadline: + try: + os.stat(fname) + except OSError as e: + if e.winerror not in ignored_errors: + raise + finally: + p.terminate() + + @os_helper.skip_unless_symlink class NonLocalSymlinkTests(unittest.TestCase): From cfc3f2bdeecd2a78b09e9561d96a51d68e15ecba Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sun, 13 Mar 2022 22:12:28 +0200 Subject: [PATCH 03/15] Add myself to Misc/ACKS --- Misc/ACKS | 1 + 1 file changed, 1 insertion(+) diff --git a/Misc/ACKS b/Misc/ACKS index 84fcb322d4086a..307a72091fe7d0 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -1701,6 +1701,7 @@ Anthony Starks David Steele Oliver Steele Greg Stein +Itai Steinherz Marek Stepniowski Baruch Sterin Chris Stern From cb4896b4d91a3a1b296161d397f9563b9a3aa499 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sun, 13 Mar 2022 20:35:42 +0000 Subject: [PATCH 04/15] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst diff --git a/Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst b/Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst new file mode 100644 index 00000000000000..9c7a03890fa3c9 --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst @@ -0,0 +1 @@ +Fix race condition between :func:`os.stat` and unlinking a file on Windows, by gracefully handling expected errors returned by ``FindFirstFileW()`` in ``win32_xstat_impl``. From b78e22c96dd5d28178e42c4b17e0a2f4c13e050e Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sun, 13 Mar 2022 22:41:04 +0200 Subject: [PATCH 05/15] Remove resolved TODO --- Lib/test/test_os.py | 1 - Modules/posixmodule.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 8c990c4b4c1ea2..d430507a77e226 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2884,7 +2884,6 @@ def test_stat_unlink_race(self): # TODO: Figure out why can't `fname` be simply `os_helper.TESTFN`? fname = os.path.join(os.environ['TEMP'], os_helper.TESTFN + '_46785') self.addCleanup(os_helper.unlink, fname) - # TODO: Think about putting this in an actual file? command = '''if 1: import os import sys diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index ac285d450b3f3c..54afa2b0375a33 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -1896,7 +1896,7 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result, case ERROR_NOT_READY: case ERROR_BAD_NET_NAME: break; - // restore the error from CreateFileW() + /* Restore the error from CreateFileW(). */ default: SetLastError(error); } From 918e1bac55a806baa34ecd5f9d243217bc6e353f Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sun, 13 Mar 2022 22:51:52 +0200 Subject: [PATCH 06/15] Resolve all TODOs --- Lib/test/test_os.py | 15 +++++++-------- Modules/posixmodule.c | 1 - 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index d430507a77e226..d10292e9cee8ee 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2881,21 +2881,20 @@ def test_stat_unlink_race(self): # directory are subsequently unlinked, or because the volume or # share are no longer available, then the original permission error # should not be restored. - # TODO: Figure out why can't `fname` be simply `os_helper.TESTFN`? - fname = os.path.join(os.environ['TEMP'], os_helper.TESTFN + '_46785') - self.addCleanup(os_helper.unlink, fname) + filename = os_helper.TESTFN + self.addCleanup(os_helper.unlink, filename) command = '''if 1: import os import sys - fname = sys.argv[1] + filename = sys.argv[1] while True: try: - with open(fname, "w") as f: + with open(filename, "w") as f: pass except OSError: pass try: - os.remove(fname) + os.remove(filename) except OSError: pass ''' @@ -2906,12 +2905,12 @@ def test_stat_unlink_race(self): 67, # ERROR_BAD_NET_NAME ) deadline = time.time() + 5 - p = subprocess.Popen([sys.executable, '-c', command, fname]) + p = subprocess.Popen([sys.executable, '-c', command, filename]) try: while time.time() < deadline: try: - os.stat(fname) + os.stat(filename) except OSError as e: if e.winerror not in ignored_errors: raise diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 54afa2b0375a33..2f839b33be7e1e 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -1891,7 +1891,6 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result, DWORD dir_error = GetLastError(); switch (GetLastError()) { case ERROR_FILE_NOT_FOUND: - // TODO: Check why should I catch these? case ERROR_PATH_NOT_FOUND: case ERROR_NOT_READY: case ERROR_BAD_NET_NAME: From d576c3ff9ae0f5931145f68139d55f98ba26f81d Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sun, 13 Mar 2022 23:12:27 +0200 Subject: [PATCH 07/15] Document caught error codes --- Modules/posixmodule.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 2f839b33be7e1e..4d18b1d9b15b57 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -1890,10 +1890,10 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result, /* Cannot read the parent directory. */ DWORD dir_error = GetLastError(); switch (GetLastError()) { - case ERROR_FILE_NOT_FOUND: - case ERROR_PATH_NOT_FOUND: - case ERROR_NOT_READY: - case ERROR_BAD_NET_NAME: + case ERROR_FILE_NOT_FOUND: /* File cannot be found */ + case ERROR_PATH_NOT_FOUND: /* File parent directory cannot be found */ + case ERROR_NOT_READY: /* Drive exists but unavailable */ + case ERROR_BAD_NET_NAME: /* Remote drive unavailable */ break; /* Restore the error from CreateFileW(). */ default: From a8fb3f79bb43b7f765455ed673835f51f0dda40c Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sun, 13 Mar 2022 23:32:11 +0200 Subject: [PATCH 08/15] Remove duplicate call to `GetLastError()` --- Modules/posixmodule.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 4d18b1d9b15b57..d449ed9f8a9e87 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -1888,7 +1888,6 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result, /* Try reading the parent directory. */ if (!attributes_from_dir(path, &fileInfo, &tagInfo.ReparseTag)) { /* Cannot read the parent directory. */ - DWORD dir_error = GetLastError(); switch (GetLastError()) { case ERROR_FILE_NOT_FOUND: /* File cannot be found */ case ERROR_PATH_NOT_FOUND: /* File parent directory cannot be found */ From 88fb1fe5ef01739304dc1e9ee8e7f2fb93d4292f Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Mon, 14 Mar 2022 22:14:01 +0200 Subject: [PATCH 09/15] CR improvement --- Lib/test/test_os.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index d10292e9cee8ee..e98c1d19cddd21 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2898,12 +2898,6 @@ def test_stat_unlink_race(self): except OSError: pass ''' - ignored_errors = ( - 2, # ERROR_FILE_NOT_FOUND - 3, # ERROR_PATH_NOT_FOUND - 21, # ERROR_NOT_READY - 67, # ERROR_BAD_NET_NAME - ) deadline = time.time() + 5 p = subprocess.Popen([sys.executable, '-c', command, filename]) @@ -2911,9 +2905,12 @@ def test_stat_unlink_race(self): while time.time() < deadline: try: os.stat(filename) - except OSError as e: - if e.winerror not in ignored_errors: - raise + # Note that `ERROR_NOT_READY`, which should also not be + # ignored, results in a `PermissionError`. That is not caught + # here, as only the behavior for `ERROR_FILE_NOT_FOUND` is + # checked in this test. + except FileNotFoundError: + pass finally: p.terminate() From 5ac6f47a32a2980c640abb6f84026c3bf094ad99 Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Fri, 18 Mar 2022 15:18:18 +0200 Subject: [PATCH 10/15] CR improvements --- Lib/test/test_os.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e98c1d19cddd21..a2410e364be5cc 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -23,6 +23,7 @@ import sys import sysconfig import tempfile +import textwrap import threading import time import types @@ -2883,11 +2884,14 @@ def test_stat_unlink_race(self): # should not be restored. filename = os_helper.TESTFN self.addCleanup(os_helper.unlink, filename) - command = '''if 1: + deadline = time.time() + 5 + command = textwrap.dedent("""\ import os import sys + import time filename = sys.argv[1] - while True: + deadline = float(sys.argv[2]) + while time.time() < deadline: try: with open(filename, "w") as f: pass @@ -2897,11 +2901,9 @@ def test_stat_unlink_race(self): os.remove(filename) except OSError: pass - ''' - deadline = time.time() + 5 - p = subprocess.Popen([sys.executable, '-c', command, filename]) + """) - try: + with subprocess.Popen([sys.executable, '-c', command, filename, str(deadline)]) as proc: while time.time() < deadline: try: os.stat(filename) @@ -2911,8 +2913,6 @@ def test_stat_unlink_race(self): # checked in this test. except FileNotFoundError: pass - finally: - p.terminate() @os_helper.skip_unless_symlink From b21208767007abba412c13cfff535289b05df056 Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Fri, 18 Mar 2022 15:32:05 +0200 Subject: [PATCH 11/15] Remove unused variable --- Lib/test/test_os.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index a2410e364be5cc..8e4639ccd7bcdf 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2903,7 +2903,7 @@ def test_stat_unlink_race(self): pass """) - with subprocess.Popen([sys.executable, '-c', command, filename, str(deadline)]) as proc: + with subprocess.Popen([sys.executable, '-c', command, filename, str(deadline)]): while time.time() < deadline: try: os.stat(filename) From 766939745896389b38e2212878730a334f26d6cf Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Fri, 18 Mar 2022 15:52:16 +0200 Subject: [PATCH 12/15] Terminate test thread at end of test --- Lib/test/test_os.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 8e4639ccd7bcdf..98d62abda1ac8e 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2903,7 +2903,7 @@ def test_stat_unlink_race(self): pass """) - with subprocess.Popen([sys.executable, '-c', command, filename, str(deadline)]): + with subprocess.Popen([sys.executable, '-c', command, filename, str(deadline)]) as proc: while time.time() < deadline: try: os.stat(filename) @@ -2913,6 +2913,10 @@ def test_stat_unlink_race(self): # checked in this test. except FileNotFoundError: pass + try: + proc.wait(1) + except subprocess.TimeoutExpired: + proc.terminate() @os_helper.skip_unless_symlink From 1963993abdbc38361e91c0abd597d3e80b2b141a Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Fri, 18 Mar 2022 16:41:47 +0200 Subject: [PATCH 13/15] Add test for race between rmdir and stat --- Lib/test/test_os.py | 49 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 98d62abda1ac8e..e6e0f6a823c57f 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2889,8 +2889,10 @@ def test_stat_unlink_race(self): import os import sys import time + filename = sys.argv[1] deadline = float(sys.argv[2]) + while time.time() < deadline: try: with open(filename, "w") as f: @@ -2907,12 +2909,49 @@ def test_stat_unlink_race(self): while time.time() < deadline: try: os.stat(filename) - # Note that `ERROR_NOT_READY`, which should also not be - # ignored, results in a `PermissionError`. That is not caught - # here, as only the behavior for `ERROR_FILE_NOT_FOUND` is - # checked in this test. - except FileNotFoundError: + except FileNotFoundError as e: + assert e.winerror == 2 # ERROR_FILE_NOT_FOUND + try: + proc.wait(1) + except subprocess.TimeoutExpired: + proc.terminate() + + @support.requires_subprocess() + def test_stat_rmdir_race(self): + # bpo-46785: same test as `test_stat_unlink_race`, excpet that this + # tests the case where the directory of the file is removed, not the + # file itself. + path = os_helper.TESTFN + filename = os.path.join(os_helper.TESTFN, 'f1') + self.addCleanup(os_helper.rmtree, path) + deadline = time.time() + 5 + command = textwrap.dedent("""\ + import shutil + import sys + import time + + filename = sys.argv[1] + path = sys.argv[2] + deadline = float(sys.argv[3]) + + while time.time() < deadline: + try: + with open(filename, "w") as f: + pass + except OSError: + pass + try: + shutil.rmtree(path) + except OSError: pass + """) + + with subprocess.Popen([sys.executable, '-c', command, filename, path, str(deadline)]) as proc: + while time.time() < deadline: + try: + os.stat(filename) + except FileNotFoundError as e: + assert e.winerror == 3 # ERROR_PATH_NOT_FOUND try: proc.wait(1) except subprocess.TimeoutExpired: From 4c0f3423951b00aaeba0d52cbd3800e60af5fae0 Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Fri, 18 Mar 2022 18:32:43 +0200 Subject: [PATCH 14/15] Improve bugfix description --- .../next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst b/Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst index 9c7a03890fa3c9..0a87abd77c8ffe 100644 --- a/Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst +++ b/Misc/NEWS.d/next/Windows/2022-03-13-20-35-41.bpo-46785.Pnknyl.rst @@ -1 +1 @@ -Fix race condition between :func:`os.stat` and unlinking a file on Windows, by gracefully handling expected errors returned by ``FindFirstFileW()`` in ``win32_xstat_impl``. +Fix race condition between :func:`os.stat` and unlinking a file on Windows, by using errors codes returned by ``FindFirstFileW()`` when appropriate in ``win32_xstat_impl``. From 765194c4fdefe95317333d4f1f1619dcbda204f0 Mon Sep 17 00:00:00 2001 From: Itai Steinherz Date: Sat, 19 Mar 2022 16:29:15 +0200 Subject: [PATCH 15/15] Remove new test --- Lib/test/test_os.py | 41 ----------------------------------------- 1 file changed, 41 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e6e0f6a823c57f..9f43bff92857d1 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2916,47 +2916,6 @@ def test_stat_unlink_race(self): except subprocess.TimeoutExpired: proc.terminate() - @support.requires_subprocess() - def test_stat_rmdir_race(self): - # bpo-46785: same test as `test_stat_unlink_race`, excpet that this - # tests the case where the directory of the file is removed, not the - # file itself. - path = os_helper.TESTFN - filename = os.path.join(os_helper.TESTFN, 'f1') - self.addCleanup(os_helper.rmtree, path) - deadline = time.time() + 5 - command = textwrap.dedent("""\ - import shutil - import sys - import time - - filename = sys.argv[1] - path = sys.argv[2] - deadline = float(sys.argv[3]) - - while time.time() < deadline: - try: - with open(filename, "w") as f: - pass - except OSError: - pass - try: - shutil.rmtree(path) - except OSError: - pass - """) - - with subprocess.Popen([sys.executable, '-c', command, filename, path, str(deadline)]) as proc: - while time.time() < deadline: - try: - os.stat(filename) - except FileNotFoundError as e: - assert e.winerror == 3 # ERROR_PATH_NOT_FOUND - try: - proc.wait(1) - except subprocess.TimeoutExpired: - proc.terminate() - @os_helper.skip_unless_symlink class NonLocalSymlinkTests(unittest.TestCase):