From d7a4f366b62d8e77a8c87acc101d5b051b8b2268 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 20:52:06 +0000 Subject: [PATCH 1/8] gh-111877: Update test to fix intermittent failures --- 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 98b30d2108a1a1..6bfd8cd5924f7c 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3087,7 +3087,7 @@ def test_stat_unlink_race(self): @support.requires_subprocess() def test_stat_inaccessible_file(self): - filename = os_helper.TESTFN + filename = os.path.abspath(os_helper.TESTFN) ICACLS = os.path.expandvars(r"%SystemRoot%\System32\icacls.exe") with open(filename, "wb") as f: From ba1930007aa60516b2b205cfef31a8b6a512aa19 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 21:15:47 +0000 Subject: [PATCH 2/8] Add extra output for diagnosis --- Lib/test/test_os.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 6bfd8cd5924f7c..d583345fdf9756 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3097,8 +3097,9 @@ def test_stat_inaccessible_file(self): try: # Remove all permissions from the file - subprocess.check_output([ICACLS, filename, "/inheritance:r"], + out = subprocess.check_output([ICACLS, filename, "/inheritance:r"], stderr=subprocess.STDOUT) + print(out) except subprocess.CalledProcessError as ex: if support.verbose: print(ICACLS, filename, "/inheritance:r", "failed.") From bf2aa36e13e7b3c8ab401ae360aa0248df825e3d Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 21:26:56 +0000 Subject: [PATCH 3/8] Add extra output for diagnosis --- 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 d583345fdf9756..f6b10f599c5394 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3099,7 +3099,7 @@ def test_stat_inaccessible_file(self): # Remove all permissions from the file out = subprocess.check_output([ICACLS, filename, "/inheritance:r"], stderr=subprocess.STDOUT) - print(out) + print(out.decode('ascii', 'backslashreplace')) except subprocess.CalledProcessError as ex: if support.verbose: print(ICACLS, filename, "/inheritance:r", "failed.") From eeab27d3b2a93b307bc65a9e7cf0d8ea80db2e20 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 22:04:36 +0000 Subject: [PATCH 4/8] More logging output --- Lib/test/test_os.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index f6b10f599c5394..2820ab96d93c8c 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3095,11 +3095,15 @@ def test_stat_inaccessible_file(self): stat1 = os.stat(filename) + # Read permissions back. This will help diagnose in case of failure + out = subprocess.check_output([ICACLS, filename], stderr=subprocess.STDOUT) + if support.verbose: + print(out.decode("oem", "backslashreplace")) + try: # Remove all permissions from the file - out = subprocess.check_output([ICACLS, filename, "/inheritance:r"], + subprocess.check_output([ICACLS, filename, "/inheritance:r"], stderr=subprocess.STDOUT) - print(out.decode('ascii', 'backslashreplace')) except subprocess.CalledProcessError as ex: if support.verbose: print(ICACLS, filename, "/inheritance:r", "failed.") @@ -3119,6 +3123,11 @@ def cleanup(): self.addCleanup(cleanup) + # Read permissions back. This will help diagnose in case of failure + out = subprocess.check_output([ICACLS, filename], stderr=subprocess.STDOUT) + if support.verbose: + print(out.decode("oem", "backslashreplace")) + if support.verbose: print("File:", filename) print("stat with access:", stat1) From e6623e6654c54f00fd4ed9ea5e0ae729f3bf5d9a Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 22:27:58 +0000 Subject: [PATCH 5/8] Use our own filename --- Lib/test/test_os.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 2820ab96d93c8c..1e606dc9f89a7a 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3087,7 +3087,7 @@ def test_stat_unlink_race(self): @support.requires_subprocess() def test_stat_inaccessible_file(self): - filename = os.path.abspath(os_helper.TESTFN) + filename = os.path.abspath("test_stat_inaccessible_file.txt") ICACLS = os.path.expandvars(r"%SystemRoot%\System32\icacls.exe") with open(filename, "wb") as f: @@ -3123,11 +3123,6 @@ def cleanup(): self.addCleanup(cleanup) - # Read permissions back. This will help diagnose in case of failure - out = subprocess.check_output([ICACLS, filename], stderr=subprocess.STDOUT) - if support.verbose: - print(out.decode("oem", "backslashreplace")) - if support.verbose: print("File:", filename) print("stat with access:", stat1) @@ -3139,6 +3134,11 @@ def cleanup(): if support.verbose: print(" without access:", stat2) + # Read permissions back. This will help diagnose in case of failure + out = subprocess.check_output([ICACLS, filename], stderr=subprocess.STDOUT) + if support.verbose: + print(out.decode("oem", "backslashreplace")) + # We cannot get st_dev/st_ino, so ensure those are 0 or else our test # is not set up correctly self.assertEqual(0, stat2.st_dev) From 6d0eac52ea19cff9dd4b93fb235f69ecf2658000 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 23:04:19 +0000 Subject: [PATCH 6/8] Alternate idea --- Lib/test/test_os.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 1e606dc9f89a7a..af4b9685cbb6d2 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3095,11 +3095,6 @@ def test_stat_inaccessible_file(self): stat1 = os.stat(filename) - # Read permissions back. This will help diagnose in case of failure - out = subprocess.check_output([ICACLS, filename], stderr=subprocess.STDOUT) - if support.verbose: - print(out.decode("oem", "backslashreplace")) - try: # Remove all permissions from the file subprocess.check_output([ICACLS, filename, "/inheritance:r"], @@ -3114,6 +3109,22 @@ def test_stat_inaccessible_file(self): pass self.skipTest("Unable to create inaccessible file") + # We should now not be able to open the file. If we can, the test isn't + # going to work. A few retries to try and catch slow updates + try: + for _ in range(5): + with open(filename, "rb") as f: + pass + time.sleep(0.5) + except PermissionError: + pass + else: + try: + os.unlink(filename) + except Exception: + pass + self.skipTest("Still had access to inaccessible file") + def cleanup(): # Give delete permission. We are the file owner, so we can do this # even though we removed all permissions earlier. @@ -3134,11 +3145,6 @@ def cleanup(): if support.verbose: print(" without access:", stat2) - # Read permissions back. This will help diagnose in case of failure - out = subprocess.check_output([ICACLS, filename], stderr=subprocess.STDOUT) - if support.verbose: - print(out.decode("oem", "backslashreplace")) - # We cannot get st_dev/st_ino, so ensure those are 0 or else our test # is not set up correctly self.assertEqual(0, stat2.st_dev) From 4ce74c28cc7b5aa1785a044be26ca3921f585341 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 23:37:52 +0000 Subject: [PATCH 7/8] Revert to original test --- Lib/test/test_os.py | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index af4b9685cbb6d2..98b30d2108a1a1 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3087,7 +3087,7 @@ def test_stat_unlink_race(self): @support.requires_subprocess() def test_stat_inaccessible_file(self): - filename = os.path.abspath("test_stat_inaccessible_file.txt") + filename = os_helper.TESTFN ICACLS = os.path.expandvars(r"%SystemRoot%\System32\icacls.exe") with open(filename, "wb") as f: @@ -3109,22 +3109,6 @@ def test_stat_inaccessible_file(self): pass self.skipTest("Unable to create inaccessible file") - # We should now not be able to open the file. If we can, the test isn't - # going to work. A few retries to try and catch slow updates - try: - for _ in range(5): - with open(filename, "rb") as f: - pass - time.sleep(0.5) - except PermissionError: - pass - else: - try: - os.unlink(filename) - except Exception: - pass - self.skipTest("Still had access to inaccessible file") - def cleanup(): # Give delete permission. We are the file owner, so we can do this # even though we removed all permissions earlier. From bf3a59e9b7e988f551644616af37927cc963a04e Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 15 Jan 2024 23:53:50 +0000 Subject: [PATCH 8/8] gh-114096: Restore privileges in _winapi.CreateJunction --- ...-01-15-23-53-25.gh-issue-114096.G-Myja.rst | 3 ++ Modules/_winapi.c | 28 ++++++++++++++----- 2 files changed, 24 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Windows/2024-01-15-23-53-25.gh-issue-114096.G-Myja.rst diff --git a/Misc/NEWS.d/next/Windows/2024-01-15-23-53-25.gh-issue-114096.G-Myja.rst b/Misc/NEWS.d/next/Windows/2024-01-15-23-53-25.gh-issue-114096.G-Myja.rst new file mode 100644 index 00000000000000..f28fc04baa7fb1 --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2024-01-15-23-53-25.gh-issue-114096.G-Myja.rst @@ -0,0 +1,3 @@ +Process privileges that are activated for creating directory junctions are +now restored afterwards, avoiding behaviour changes in other parts of the +program. diff --git a/Modules/_winapi.c b/Modules/_winapi.c index a26850e825b492..26302b559817b3 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -532,7 +532,12 @@ _winapi_CreateJunction_impl(PyObject *module, LPCWSTR src_path, { /* Privilege adjustment */ HANDLE token = NULL; - TOKEN_PRIVILEGES tp; + struct { + TOKEN_PRIVILEGES base; + /* overallocate by a few array elements */ + LUID_AND_ATTRIBUTES privs[4]; + } tp, previousTp; + int previousTpSize = 0; /* Reparse data buffer */ const USHORT prefix_len = 4; @@ -556,17 +561,21 @@ _winapi_CreateJunction_impl(PyObject *module, LPCWSTR src_path, /* Adjust privileges to allow rewriting directory entry as a junction point. */ - if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES, &token)) + if (!OpenProcessToken(GetCurrentProcess(), + TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &token)) { goto cleanup; + } - if (!LookupPrivilegeValue(NULL, SE_RESTORE_NAME, &tp.Privileges[0].Luid)) + if (!LookupPrivilegeValue(NULL, SE_RESTORE_NAME, &tp.base.Privileges[0].Luid)) { goto cleanup; + } - tp.PrivilegeCount = 1; - tp.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED; - if (!AdjustTokenPrivileges(token, FALSE, &tp, sizeof(TOKEN_PRIVILEGES), - NULL, NULL)) + tp.base.PrivilegeCount = 1; + tp.base.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED; + if (!AdjustTokenPrivileges(token, FALSE, &tp.base, sizeof(previousTp), + &previousTp.base, &previousTpSize)) { goto cleanup; + } if (GetFileAttributesW(src_path) == INVALID_FILE_ATTRIBUTES) goto cleanup; @@ -647,6 +656,11 @@ _winapi_CreateJunction_impl(PyObject *module, LPCWSTR src_path, cleanup: ret = GetLastError(); + if (previousTpSize) { + AdjustTokenPrivileges(token, FALSE, &previousTp.base, previousTpSize, + NULL, NULL); + } + if (token != NULL) CloseHandle(token); if (junction != NULL)