From c08870cb410e3edce0fc34849bcbf690b2b8fe6b Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 25 Jan 2024 23:00:01 +0000 Subject: [PATCH 1/3] gh-114435: Add test skip when running as admin. Also makes _winapi.CreateFile unconditionally use Unicode. --- Lib/test/test_os.py | 17 +++++++++++++++++ Modules/_winapi.c | 17 +++++++++-------- Modules/clinic/_winapi.c.h | 13 ++++++++----- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index ed1f304c6c8cac..db9f6cd9f2fe22 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3122,6 +3122,23 @@ def cleanup(): print("File:", filename) print("stat with access:", stat1) + # We should now not be able to open the file. + # If we can, the test isn't going to be useful. + try: + _winapi.CloseHandle(_winapi.CreateFile( + filename, + 0x80, # FILE_READ_ATTRIBUTES + 0, + 0, + _winapi.OPEN_EXISTING, + 0x02000000, # FILE_FLAG_BACKUP_SEMANTICS + 0, + )) + except PermissionError: + pass + else: + self.skipTest("Still had access to inaccessible file") + # First test - we shouldn't raise here, because we still have access to # the directory and can extract enough information from its metadata. stat2 = os.stat(filename) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 26302b559817b3..5e5eb123c4ccff 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -441,7 +441,7 @@ _winapi_ConnectNamedPipe_impl(PyObject *module, HANDLE handle, /*[clinic input] _winapi.CreateFile -> HANDLE - file_name: LPCTSTR + file_name: LPCWSTR desired_access: DWORD share_mode: DWORD security_attributes: LPSECURITY_ATTRIBUTES @@ -452,12 +452,12 @@ _winapi.CreateFile -> HANDLE [clinic start generated code]*/ static HANDLE -_winapi_CreateFile_impl(PyObject *module, LPCTSTR file_name, +_winapi_CreateFile_impl(PyObject *module, LPCWSTR file_name, DWORD desired_access, DWORD share_mode, LPSECURITY_ATTRIBUTES security_attributes, DWORD creation_disposition, DWORD flags_and_attributes, HANDLE template_file) -/*[clinic end generated code: output=417ddcebfc5a3d53 input=6423c3e40372dbd5]*/ +/*[clinic end generated code: output=818c811e5e04d550 input=1fa870ed1c2e3d69]*/ { HANDLE handle; @@ -468,14 +468,15 @@ _winapi_CreateFile_impl(PyObject *module, LPCTSTR file_name, } Py_BEGIN_ALLOW_THREADS - handle = CreateFile(file_name, desired_access, - share_mode, security_attributes, - creation_disposition, - flags_and_attributes, template_file); + handle = CreateFileW(file_name, desired_access, + share_mode, security_attributes, + creation_disposition, + flags_and_attributes, template_file); Py_END_ALLOW_THREADS - if (handle == INVALID_HANDLE_VALUE) + if (handle == INVALID_HANDLE_VALUE) { PyErr_SetFromWindowsErr(0); + } return handle; } diff --git a/Modules/clinic/_winapi.c.h b/Modules/clinic/_winapi.c.h index 3a3231c051ef71..d1052f38919dde 100644 --- a/Modules/clinic/_winapi.c.h +++ b/Modules/clinic/_winapi.c.h @@ -162,7 +162,7 @@ PyDoc_STRVAR(_winapi_CreateFile__doc__, {"CreateFile", _PyCFunction_CAST(_winapi_CreateFile), METH_FASTCALL, _winapi_CreateFile__doc__}, static HANDLE -_winapi_CreateFile_impl(PyObject *module, LPCTSTR file_name, +_winapi_CreateFile_impl(PyObject *module, LPCWSTR file_name, DWORD desired_access, DWORD share_mode, LPSECURITY_ATTRIBUTES security_attributes, DWORD creation_disposition, @@ -172,7 +172,7 @@ static PyObject * _winapi_CreateFile(PyObject *module, PyObject *const *args, Py_ssize_t nargs) { PyObject *return_value = NULL; - LPCTSTR file_name; + LPCWSTR file_name = NULL; DWORD desired_access; DWORD share_mode; LPSECURITY_ATTRIBUTES security_attributes; @@ -181,8 +181,8 @@ _winapi_CreateFile(PyObject *module, PyObject *const *args, Py_ssize_t nargs) HANDLE template_file; HANDLE _return_value; - if (!_PyArg_ParseStack(args, nargs, "skk" F_POINTER "kk" F_HANDLE ":CreateFile", - &file_name, &desired_access, &share_mode, &security_attributes, &creation_disposition, &flags_and_attributes, &template_file)) { + if (!_PyArg_ParseStack(args, nargs, "O&kk" F_POINTER "kk" F_HANDLE ":CreateFile", + _PyUnicode_WideCharString_Converter, &file_name, &desired_access, &share_mode, &security_attributes, &creation_disposition, &flags_and_attributes, &template_file)) { goto exit; } _return_value = _winapi_CreateFile_impl(module, file_name, desired_access, share_mode, security_attributes, creation_disposition, flags_and_attributes, template_file); @@ -195,6 +195,9 @@ _winapi_CreateFile(PyObject *module, PyObject *const *args, Py_ssize_t nargs) return_value = HANDLE_TO_PYNUM(_return_value); exit: + /* Cleanup for file_name */ + PyMem_Free((void *)file_name); + return return_value; } @@ -1479,4 +1482,4 @@ _winapi_CopyFile2(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyO return return_value; } -/*[clinic end generated code: output=e1a9908bb82a6379 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=2350d4f2275d3a6f input=a9049054013a1b77]*/ From 9596d288a55727e92a91f21e67c2799186dcedd7 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 26 Jan 2024 14:15:51 +0000 Subject: [PATCH 2/3] Replace skip check with another valid value --- Lib/test/test_os.py | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index db9f6cd9f2fe22..e6f0dfde8cb4ae 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3122,23 +3122,6 @@ def cleanup(): print("File:", filename) print("stat with access:", stat1) - # We should now not be able to open the file. - # If we can, the test isn't going to be useful. - try: - _winapi.CloseHandle(_winapi.CreateFile( - filename, - 0x80, # FILE_READ_ATTRIBUTES - 0, - 0, - _winapi.OPEN_EXISTING, - 0x02000000, # FILE_FLAG_BACKUP_SEMANTICS - 0, - )) - except PermissionError: - pass - else: - self.skipTest("Still had access to inaccessible file") - # First test - we shouldn't raise here, because we still have access to # the directory and can extract enough information from its metadata. stat2 = os.stat(filename) @@ -3146,10 +3129,9 @@ def cleanup(): if support.verbose: print(" without access:", stat2) - # 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) - self.assertEqual(0, stat2.st_ino) + # We may not get st_dev/st_ino, so ensure those are 0 or match + self.assertIn(stat2.st_dev, (0, stat1.st_dev)) + self.assertIn(stat2.st_ino, (0, stat1.st_ino)) # st_mode and st_size should match (for a normal file, at least) self.assertEqual(stat1.st_mode, stat2.st_mode) From 2ba91219ea6ca975be8bd0ff70da9ad63b03b3c0 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 26 Jan 2024 16:45:08 +0000 Subject: [PATCH 3/3] Revert gh-114435's change --- Lib/test/test_os.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e6f0dfde8cb4ae..ed1f304c6c8cac 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -3129,9 +3129,10 @@ def cleanup(): if support.verbose: print(" without access:", stat2) - # We may not get st_dev/st_ino, so ensure those are 0 or match - self.assertIn(stat2.st_dev, (0, stat1.st_dev)) - self.assertIn(stat2.st_ino, (0, stat1.st_ino)) + # 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) + self.assertEqual(0, stat2.st_ino) # st_mode and st_size should match (for a normal file, at least) self.assertEqual(stat1.st_mode, stat2.st_mode)