From b9c4dbe25570e1bdeaa52de436438999524e7c6c Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Mon, 27 Mar 2023 13:05:45 -0700 Subject: [PATCH 01/15] pythongh-102765: added _Py_GetFileInformationByName to isdir --- Modules/posixmodule.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index e38caf7cc0abee..5bd3a76877671c 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4789,6 +4789,8 @@ os__path_isdir_impl(PyObject *module, PyObject *path) FILE_BASIC_INFO info; path_t _path = PATH_T_INITIALIZE("isdir", "path", 0, 1); int result; + BOOL slow_path = TRUE; + FILE_STAT_BASIC_INFORMATION statInfo; if (!path_converter(path, &_path)) { path_cleanup(&_path); @@ -4800,15 +4802,36 @@ os__path_isdir_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS + if(_path.wide){ + if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + &statInfo, sizeof(statInfo))) { + if (// Cannot use fast path for reparse points ... + !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) + ) { + slow_path = FALSE; + } + } else { + switch(GetLastError()) { + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: + case ERROR_NOT_READY: + case ERROR_BAD_NET_NAME: + /* These errors aren't worth retrying with the slow path */ + slow_path = FALSE; + case ERROR_NOT_SUPPORTED: + /* indicates the API couldn't be loaded */ + break; + } + } if (_path.fd != -1) { hfile = _Py_get_osfhandle_noraise(_path.fd); close_file = FALSE; } - else { + else if(slow_path){ hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); } - if (hfile != INVALID_HANDLE_VALUE) { + if (slow_path && hfile != INVALID_HANDLE_VALUE) { if (GetFileInformationByHandleEx(hfile, FileBasicInfo, &info, sizeof(info))) { From a13692241ee6c1909a8e5e2f0c36a9a10e9114f2 Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Mon, 27 Mar 2023 14:27:26 -0700 Subject: [PATCH 02/15] enable actions workflows for branch --- .github/workflows/build.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 4e5328282f1224..bbd57471c9c4e0 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -13,6 +13,7 @@ on: - '3.9' - '3.8' - '3.7' + - 'gh-102765' pull_request: branches: - 'main' From a1a3fa5c9c16dfb2c4fc40837c46f1522d32f28d Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Mon, 27 Mar 2023 14:54:10 -0700 Subject: [PATCH 03/15] pythongh-102765: add missing curly brace, fix whitespace --- Modules/posixmodule.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 5bd3a76877671c..02450cae1b4eb8 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4811,16 +4811,17 @@ os__path_isdir_impl(PyObject *module, PyObject *path) slow_path = FALSE; } } else { - switch(GetLastError()) { - case ERROR_FILE_NOT_FOUND: - case ERROR_PATH_NOT_FOUND: - case ERROR_NOT_READY: - case ERROR_BAD_NET_NAME: - /* These errors aren't worth retrying with the slow path */ - slow_path = FALSE; - case ERROR_NOT_SUPPORTED: - /* indicates the API couldn't be loaded */ - break; + switch(GetLastError()) { + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: + case ERROR_NOT_READY: + case ERROR_BAD_NET_NAME: + /* These errors aren't worth retrying with the slow path */ + slow_path = FALSE; + case ERROR_NOT_SUPPORTED: + /* indicates the API couldn't be loaded */ + break; + } } } if (_path.fd != -1) { From aa984f2da6cc8c3535f827a54c15c28af3db9c1a Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Mon, 27 Mar 2023 15:47:14 -0700 Subject: [PATCH 04/15] pythongh-102765: added _Py_GetFileInformationByName to exists & isfile --- Modules/posixmodule.c | 56 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 02450cae1b4eb8..b4b7061ee51c61 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4891,6 +4891,8 @@ os__path_isfile_impl(PyObject *module, PyObject *path) FILE_BASIC_INFO info; path_t _path = PATH_T_INITIALIZE("isfile", "path", 0, 1); int result; + BOOL slow_path = TRUE; + FILE_STAT_BASIC_INFORMATION statInfo; if (!path_converter(path, &_path)) { path_cleanup(&_path); @@ -4902,15 +4904,37 @@ os__path_isfile_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS + if(_path.wide){ + if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + &statInfo, sizeof(statInfo))) { + if (// Cannot use fast path for reparse points ... + !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) + ) { + slow_path = FALSE; + } + } else { + switch(GetLastError()) { + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: + case ERROR_NOT_READY: + case ERROR_BAD_NET_NAME: + /* These errors aren't worth retrying with the slow path */ + slow_path = FALSE; + case ERROR_NOT_SUPPORTED: + /* indicates the API couldn't be loaded */ + break; + } + } + } if (_path.fd != -1) { hfile = _Py_get_osfhandle_noraise(_path.fd); close_file = FALSE; } - else { + else if(slow_path){ hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); } - if (hfile != INVALID_HANDLE_VALUE) { + if (slow_path && hfile != INVALID_HANDLE_VALUE) { if (GetFileInformationByHandleEx(hfile, FileBasicInfo, &info, sizeof(info))) { @@ -4968,6 +4992,8 @@ os__path_exists_impl(PyObject *module, PyObject *path) BOOL close_file = TRUE; path_t _path = PATH_T_INITIALIZE("exists", "path", 0, 1); int result; + BOOL slow_path = TRUE; + FILE_STAT_BASIC_INFORMATION statInfo; if (!path_converter(path, &_path)) { path_cleanup(&_path); @@ -4979,15 +5005,37 @@ os__path_exists_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS + if(_path.wide){ + if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + &statInfo, sizeof(statInfo))) { + if (// Cannot use fast path for reparse points ... + !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) + ) { + slow_path = FALSE; + } + } else { + switch(GetLastError()) { + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: + case ERROR_NOT_READY: + case ERROR_BAD_NET_NAME: + /* These errors aren't worth retrying with the slow path */ + slow_path = FALSE; + case ERROR_NOT_SUPPORTED: + /* indicates the API couldn't be loaded */ + break; + } + } + } if (_path.fd != -1) { hfile = _Py_get_osfhandle_noraise(_path.fd); close_file = FALSE; } - else { + else if(slow_path){ hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); } - if (hfile != INVALID_HANDLE_VALUE) { + if (slow_path && hfile != INVALID_HANDLE_VALUE) { result = 1; if (close_file) { CloseHandle(hfile); From fb95b7fd4db367e4795919e753cbea1d3d86a489 Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Mon, 27 Mar 2023 22:07:50 -0700 Subject: [PATCH 05/15] pythongh-102765: added _Py_GetFileInformationByName to islink, altered slow_path logic --- Modules/posixmodule.c | 276 ++++++++++++++++++++++++------------------ 1 file changed, 155 insertions(+), 121 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index b4b7061ee51c61..d41891010d8511 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4824,43 +4824,45 @@ os__path_isdir_impl(PyObject *module, PyObject *path) } } } - if (_path.fd != -1) { - hfile = _Py_get_osfhandle_noraise(_path.fd); - close_file = FALSE; - } - else if(slow_path){ - hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, - OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); - } - if (slow_path && hfile != INVALID_HANDLE_VALUE) { - if (GetFileInformationByHandleEx(hfile, FileBasicInfo, &info, - sizeof(info))) - { - result = info.FileAttributes & FILE_ATTRIBUTE_DIRECTORY; + if (slow_path){ + if (_path.fd != -1) { + hfile = _Py_get_osfhandle_noraise(_path.fd); + close_file = FALSE; } else { - result = 0; - } - if (close_file) { - CloseHandle(hfile); + hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, + OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); } - } - else { - STRUCT_STAT st; - switch (GetLastError()) { - case ERROR_ACCESS_DENIED: - case ERROR_SHARING_VIOLATION: - case ERROR_CANT_ACCESS_FILE: - case ERROR_INVALID_PARAMETER: - if (STAT(_path.wide, &st)) { - result = 0; + if (hfile != INVALID_HANDLE_VALUE) { + if (GetFileInformationByHandleEx(hfile, FileBasicInfo, &info, + sizeof(info))) + { + result = info.FileAttributes & FILE_ATTRIBUTE_DIRECTORY; } else { - result = S_ISDIR(st.st_mode); + result = 0; + } + if (close_file) { + CloseHandle(hfile); + } + } + else { + STRUCT_STAT st; + switch (GetLastError()) { + case ERROR_ACCESS_DENIED: + case ERROR_SHARING_VIOLATION: + case ERROR_CANT_ACCESS_FILE: + case ERROR_INVALID_PARAMETER: + if (STAT(_path.wide, &st)) { + result = 0; + } + else { + result = S_ISDIR(st.st_mode); + } + break; + default: + result = 0; } - break; - default: - result = 0; } } Py_END_ALLOW_THREADS @@ -4926,43 +4928,45 @@ os__path_isfile_impl(PyObject *module, PyObject *path) } } } - if (_path.fd != -1) { - hfile = _Py_get_osfhandle_noraise(_path.fd); - close_file = FALSE; - } - else if(slow_path){ - hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, - OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); - } - if (slow_path && hfile != INVALID_HANDLE_VALUE) { - if (GetFileInformationByHandleEx(hfile, FileBasicInfo, &info, - sizeof(info))) - { - result = !(info.FileAttributes & FILE_ATTRIBUTE_DIRECTORY); + if (slow_path){ + if (_path.fd != -1) { + hfile = _Py_get_osfhandle_noraise(_path.fd); + close_file = FALSE; } else { - result = 0; + hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, + OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); } - if (close_file) { - CloseHandle(hfile); - } - } - else { - STRUCT_STAT st; - switch (GetLastError()) { - case ERROR_ACCESS_DENIED: - case ERROR_SHARING_VIOLATION: - case ERROR_CANT_ACCESS_FILE: - case ERROR_INVALID_PARAMETER: - if (STAT(_path.wide, &st)) { - result = 0; + if (hfile != INVALID_HANDLE_VALUE) { + if (GetFileInformationByHandleEx(hfile, FileBasicInfo, &info, + sizeof(info))) + { + result = !(info.FileAttributes & FILE_ATTRIBUTE_DIRECTORY); } else { - result = S_ISREG(st.st_mode); + result = 0; + } + if (close_file) { + CloseHandle(hfile); + } + } + else { + STRUCT_STAT st; + switch (GetLastError()) { + case ERROR_ACCESS_DENIED: + case ERROR_SHARING_VIOLATION: + case ERROR_CANT_ACCESS_FILE: + case ERROR_INVALID_PARAMETER: + if (STAT(_path.wide, &st)) { + result = 0; + } + else { + result = S_ISREG(st.st_mode); + } + break; + default: + result = 0; } - break; - default: - result = 0; } } Py_END_ALLOW_THREADS @@ -5027,36 +5031,38 @@ os__path_exists_impl(PyObject *module, PyObject *path) } } } - if (_path.fd != -1) { - hfile = _Py_get_osfhandle_noraise(_path.fd); - close_file = FALSE; - } - else if(slow_path){ - hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, - OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); - } - if (slow_path && hfile != INVALID_HANDLE_VALUE) { - result = 1; - if (close_file) { - CloseHandle(hfile); + if (slow_path){ + if (_path.fd != -1) { + hfile = _Py_get_osfhandle_noraise(_path.fd); + close_file = FALSE; } - } - else { - STRUCT_STAT st; - switch (GetLastError()) { - case ERROR_ACCESS_DENIED: - case ERROR_SHARING_VIOLATION: - case ERROR_CANT_ACCESS_FILE: - case ERROR_INVALID_PARAMETER: - if (STAT(_path.wide, &st)) { - result = 0; + else { + hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, + OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); + } + if (hfile != INVALID_HANDLE_VALUE) { + result = 1; + if (close_file) { + CloseHandle(hfile); } - else { - result = 1; + } + else { + STRUCT_STAT st; + switch (GetLastError()) { + case ERROR_ACCESS_DENIED: + case ERROR_SHARING_VIOLATION: + case ERROR_CANT_ACCESS_FILE: + case ERROR_INVALID_PARAMETER: + if (STAT(_path.wide, &st)) { + result = 0; + } + else { + result = 1; + } + break; + default: + result = 0; } - break; - default: - result = 0; } } Py_END_ALLOW_THREADS @@ -5087,6 +5093,8 @@ os__path_islink_impl(PyObject *module, PyObject *path) FILE_ATTRIBUTE_TAG_INFO info; path_t _path = PATH_T_INITIALIZE("islink", "path", 0, 1); int result; + BOOL slow_path = TRUE; + FILE_STAT_BASIC_INFORMATION statInfo; if (!path_converter(path, &_path)) { path_cleanup(&_path); @@ -5098,45 +5106,71 @@ os__path_islink_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS - if (_path.fd != -1) { - hfile = _Py_get_osfhandle_noraise(_path.fd); - close_file = FALSE; + if(_path.wide){ + if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + &statInfo, sizeof(statInfo))) { + if (// Cannot use fast path for reparse points ... + !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) + // ... unless it's a name surrogate (symlink) and we're not following + || IsReparseTagNameSurrogate(statInfo.ReparseTag) + ) { + slow_path = FALSE; + } + } else { + switch(GetLastError()) { + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: + case ERROR_NOT_READY: + case ERROR_BAD_NET_NAME: + /* These errors aren't worth retrying with the slow path */ + slow_path = FALSE; + case ERROR_NOT_SUPPORTED: + /* indicates the API couldn't be loaded */ + break; + } + } } - else { - hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, - OPEN_EXISTING, - FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, - NULL); - } - if (hfile != INVALID_HANDLE_VALUE) { - if (GetFileInformationByHandleEx(hfile, FileAttributeTagInfo, &info, - sizeof(info))) - { - result = (info.ReparseTag == IO_REPARSE_TAG_SYMLINK); + if (slow_path){ + if (_path.fd != -1) { + hfile = _Py_get_osfhandle_noraise(_path.fd); + close_file = FALSE; } else { - result = 0; - } - if (close_file) { - CloseHandle(hfile); + hfile = CreateFileW(_path.wide, FILE_READ_ATTRIBUTES, 0, NULL, + OPEN_EXISTING, + FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, + NULL); } - } - else { - STRUCT_STAT st; - switch (GetLastError()) { - case ERROR_ACCESS_DENIED: - case ERROR_SHARING_VIOLATION: - case ERROR_CANT_ACCESS_FILE: - case ERROR_INVALID_PARAMETER: - if (LSTAT(_path.wide, &st)) { - result = 0; + if (hfile != INVALID_HANDLE_VALUE) { + if (GetFileInformationByHandleEx(hfile, FileAttributeTagInfo, &info, + sizeof(info))) + { + result = (info.ReparseTag == IO_REPARSE_TAG_SYMLINK); } else { - result = S_ISLNK(st.st_mode); + result = 0; + } + if (close_file) { + CloseHandle(hfile); + } + } + else { + STRUCT_STAT st; + switch (GetLastError()) { + case ERROR_ACCESS_DENIED: + case ERROR_SHARING_VIOLATION: + case ERROR_CANT_ACCESS_FILE: + case ERROR_INVALID_PARAMETER: + if (LSTAT(_path.wide, &st)) { + result = 0; + } + else { + result = S_ISLNK(st.st_mode); + } + break; + default: + result = 0; } - break; - default: - result = 0; } } Py_END_ALLOW_THREADS From 83de4de5d37a689f71a8a867278a2d5655168b1a Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Tue, 28 Mar 2023 10:31:59 -0700 Subject: [PATCH 06/15] fix whitespace, remove unneeded comments --- Modules/posixmodule.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index d41891010d8511..ffb9fef6353287 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4802,12 +4802,10 @@ os__path_isdir_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS - if(_path.wide){ + if (_path.wide) { if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { - if (// Cannot use fast path for reparse points ... - !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) - ) { + if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; } } else { @@ -4824,7 +4822,7 @@ os__path_isdir_impl(PyObject *module, PyObject *path) } } } - if (slow_path){ + if (slow_path) { if (_path.fd != -1) { hfile = _Py_get_osfhandle_noraise(_path.fd); close_file = FALSE; @@ -4906,12 +4904,10 @@ os__path_isfile_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS - if(_path.wide){ + if (_path.wide) { if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { - if (// Cannot use fast path for reparse points ... - !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) - ) { + if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; } } else { @@ -4928,7 +4924,7 @@ os__path_isfile_impl(PyObject *module, PyObject *path) } } } - if (slow_path){ + if (slow_path) { if (_path.fd != -1) { hfile = _Py_get_osfhandle_noraise(_path.fd); close_file = FALSE; @@ -5009,12 +5005,10 @@ os__path_exists_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS - if(_path.wide){ + if (_path.wide) { if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { - if (// Cannot use fast path for reparse points ... - !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) - ) { + if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; } } else { @@ -5031,7 +5025,7 @@ os__path_exists_impl(PyObject *module, PyObject *path) } } } - if (slow_path){ + if (slow_path) { if (_path.fd != -1) { hfile = _Py_get_osfhandle_noraise(_path.fd); close_file = FALSE; @@ -5106,12 +5100,12 @@ os__path_islink_impl(PyObject *module, PyObject *path) } Py_BEGIN_ALLOW_THREADS - if(_path.wide){ + if (_path.wide) { if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { if (// Cannot use fast path for reparse points ... !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) - // ... unless it's a name surrogate (symlink) and we're not following + // ... unless it's a name surrogate (symlink) || IsReparseTagNameSurrogate(statInfo.ReparseTag) ) { slow_path = FALSE; @@ -5130,7 +5124,7 @@ os__path_islink_impl(PyObject *module, PyObject *path) } } } - if (slow_path){ + if (slow_path) { if (_path.fd != -1) { hfile = _Py_get_osfhandle_noraise(_path.fd); close_file = FALSE; From 1b47b639c70866b181fe7a66242da73d228cacd1 Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Tue, 28 Mar 2023 10:41:23 -0700 Subject: [PATCH 07/15] add results --- Modules/posixmodule.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index ffb9fef6353287..c459e512bdef93 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4807,6 +4807,7 @@ os__path_isdir_impl(PyObject *module, PyObject *path) &statInfo, sizeof(statInfo))) { if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; + result = statInfo.FileAttributes & FILE_ATTRIBUTE_DIRECTORY; } } else { switch(GetLastError()) { @@ -4909,6 +4910,7 @@ os__path_isfile_impl(PyObject *module, PyObject *path) &statInfo, sizeof(statInfo))) { if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; + result = !(statInfo.FileAttributes & FILE_ATTRIBUTE_DIRECTORY); } } else { switch(GetLastError()) { @@ -5010,6 +5012,10 @@ os__path_exists_impl(PyObject *module, PyObject *path) &statInfo, sizeof(statInfo))) { if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; + result = 1; + if (close_file) { + CloseHandle(hfile); + } } } else { switch(GetLastError()) { @@ -5109,6 +5115,7 @@ os__path_islink_impl(PyObject *module, PyObject *path) || IsReparseTagNameSurrogate(statInfo.ReparseTag) ) { slow_path = FALSE; + result = (statInfo.ReparseTag == IO_REPARSE_TAG_SYMLINK); } } else { switch(GetLastError()) { From 43e203d0e6a3975039b0e7b9813e81d6505de48e Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Tue, 28 Mar 2023 11:32:52 -0700 Subject: [PATCH 08/15] added result for slow_path excluding errors --- Modules/posixmodule.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index c459e512bdef93..ab169c662d7b14 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4817,6 +4817,7 @@ os__path_isdir_impl(PyObject *module, PyObject *path) case ERROR_BAD_NET_NAME: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; + result = 0; case ERROR_NOT_SUPPORTED: /* indicates the API couldn't be loaded */ break; @@ -4920,6 +4921,7 @@ os__path_isfile_impl(PyObject *module, PyObject *path) case ERROR_BAD_NET_NAME: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; + result = 0; case ERROR_NOT_SUPPORTED: /* indicates the API couldn't be loaded */ break; @@ -5025,6 +5027,7 @@ os__path_exists_impl(PyObject *module, PyObject *path) case ERROR_BAD_NET_NAME: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; + result = 0; case ERROR_NOT_SUPPORTED: /* indicates the API couldn't be loaded */ break; @@ -5125,6 +5128,7 @@ os__path_islink_impl(PyObject *module, PyObject *path) case ERROR_BAD_NET_NAME: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; + result = 0; case ERROR_NOT_SUPPORTED: /* indicates the API couldn't be loaded */ break; From 0d373d11a29fed34814c250c3d95cd9feac0528d Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Tue, 28 Mar 2023 15:34:27 -0700 Subject: [PATCH 09/15] skip isdir/isfile slow path if not a dir/file --- Modules/posixmodule.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index ab169c662d7b14..c996bbcca832ab 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4808,6 +4808,9 @@ os__path_isdir_impl(PyObject *module, PyObject *path) if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; result = statInfo.FileAttributes & FILE_ATTRIBUTE_DIRECTORY; + } else if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_DIRECTORY)) { + slow_path = FALSE; + result = 0; } } else { switch(GetLastError()) { @@ -4912,6 +4915,9 @@ os__path_isfile_impl(PyObject *module, PyObject *path) if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; result = !(statInfo.FileAttributes & FILE_ATTRIBUTE_DIRECTORY); + } else if (statInfo.FileAttributes & FILE_ATTRIBUTE_DIRECTORY) { + slow_path = FALSE; + result = 0; } } else { switch(GetLastError()) { From 427811364a5a855d94ca94470f0a061d6e229872 Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Wed, 29 Mar 2023 01:53:57 -0700 Subject: [PATCH 10/15] remove extra close_file --- Modules/posixmodule.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index c996bbcca832ab..31b52f1908685b 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5021,9 +5021,6 @@ os__path_exists_impl(PyObject *module, PyObject *path) if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; result = 1; - if (close_file) { - CloseHandle(hfile); - } } } else { switch(GetLastError()) { From 72ed31743404015274a443cee14fcb5d4743be5f Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Mon, 3 Apr 2023 11:35:17 -0700 Subject: [PATCH 11/15] change path -> _path.wide --- Modules/posixmodule.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 31b52f1908685b..fe17ebc58dd789 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4803,7 +4803,7 @@ os__path_isdir_impl(PyObject *module, PyObject *path) Py_BEGIN_ALLOW_THREADS if (_path.wide) { - if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + if (_Py_GetFileInformationByName(_path.wide, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; @@ -4910,7 +4910,7 @@ os__path_isfile_impl(PyObject *module, PyObject *path) Py_BEGIN_ALLOW_THREADS if (_path.wide) { - if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + if (_Py_GetFileInformationByName(_path.wide, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; @@ -5016,7 +5016,7 @@ os__path_exists_impl(PyObject *module, PyObject *path) Py_BEGIN_ALLOW_THREADS if (_path.wide) { - if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + if (_Py_GetFileInformationByName(_path.wide, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { if (!(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { slow_path = FALSE; @@ -5113,7 +5113,7 @@ os__path_islink_impl(PyObject *module, PyObject *path) Py_BEGIN_ALLOW_THREADS if (_path.wide) { - if (_Py_GetFileInformationByName(path, FileStatBasicByNameInfo, + if (_Py_GetFileInformationByName(_path.wide, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { if (// Cannot use fast path for reparse points ... !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) From 94f8eebf63e29fff5a14774dbb3447afe0df1636 Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Wed, 12 Apr 2023 12:04:36 -0700 Subject: [PATCH 12/15] revert branch addition in actions workflow --- .github/workflows/build.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bbd57471c9c4e0..4e5328282f1224 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -13,7 +13,6 @@ on: - '3.9' - '3.8' - '3.7' - - 'gh-102765' pull_request: branches: - 'main' From 5aa3e8a1d8fea373bb4d5e136de36053a93c572d Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Thu, 13 Apr 2023 10:42:54 -0700 Subject: [PATCH 13/15] Excluded additional errors from slow fallback path --- Modules/posixmodule.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 2080f5d83994d5..cf151d3bdf8512 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4818,6 +4818,10 @@ os__path_isdir_impl(PyObject *module, PyObject *path) case ERROR_PATH_NOT_FOUND: case ERROR_NOT_READY: case ERROR_BAD_NET_NAME: + case ERROR_BAD_NETPATH: + case ERROR_BAD_PATHNAME: + case ERROR_INVALID_NAME: + case ERROR_FILENAME_EXCED_RANGE: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; result = 0; @@ -4925,6 +4929,10 @@ os__path_isfile_impl(PyObject *module, PyObject *path) case ERROR_PATH_NOT_FOUND: case ERROR_NOT_READY: case ERROR_BAD_NET_NAME: + case ERROR_BAD_NETPATH: + case ERROR_BAD_PATHNAME: + case ERROR_INVALID_NAME: + case ERROR_FILENAME_EXCED_RANGE: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; result = 0; @@ -5028,6 +5036,10 @@ os__path_exists_impl(PyObject *module, PyObject *path) case ERROR_PATH_NOT_FOUND: case ERROR_NOT_READY: case ERROR_BAD_NET_NAME: + case ERROR_BAD_NETPATH: + case ERROR_BAD_PATHNAME: + case ERROR_INVALID_NAME: + case ERROR_FILENAME_EXCED_RANGE: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; result = 0; @@ -5129,6 +5141,10 @@ os__path_islink_impl(PyObject *module, PyObject *path) case ERROR_PATH_NOT_FOUND: case ERROR_NOT_READY: case ERROR_BAD_NET_NAME: + case ERROR_BAD_NETPATH: + case ERROR_BAD_PATHNAME: + case ERROR_INVALID_NAME: + case ERROR_FILENAME_EXCED_RANGE: /* These errors aren't worth retrying with the slow path */ slow_path = FALSE; result = 0; From 333850313ddd84309930b5f3585b30b8b2ea9cea Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Thu, 13 Apr 2023 12:36:31 -0700 Subject: [PATCH 14/15] Disable slow path in islink on any successful GetFileInformationByName result Co-authored-by: Eryk Sun --- Modules/posixmodule.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index cf151d3bdf8512..cd3c67cc51ee63 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5127,14 +5127,13 @@ os__path_islink_impl(PyObject *module, PyObject *path) if (_path.wide) { if (_Py_GetFileInformationByName(_path.wide, FileStatBasicByNameInfo, &statInfo, sizeof(statInfo))) { - if (// Cannot use fast path for reparse points ... - !(statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) - // ... unless it's a name surrogate (symlink) - || IsReparseTagNameSurrogate(statInfo.ReparseTag) - ) { - slow_path = FALSE; + slow_path = FALSE; + if (statInfo.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) { result = (statInfo.ReparseTag == IO_REPARSE_TAG_SYMLINK); } + else { + result = 0; + } } else { switch(GetLastError()) { case ERROR_FILE_NOT_FOUND: From 9eb3e37990fbb8f3c5b32f16dedebad8a0cddc4e Mon Sep 17 00:00:00 2001 From: Finn Womack Date: Thu, 20 Apr 2023 10:20:10 -0700 Subject: [PATCH 15/15] move error switch into static inline function --- Include/internal/pycore_fileutils_windows.h | 18 ++++++ Modules/posixmodule.c | 64 ++------------------- 2 files changed, 22 insertions(+), 60 deletions(-) diff --git a/Include/internal/pycore_fileutils_windows.h b/Include/internal/pycore_fileutils_windows.h index 9bc7feb8cecd01..e804d385e76708 100644 --- a/Include/internal/pycore_fileutils_windows.h +++ b/Include/internal/pycore_fileutils_windows.h @@ -75,6 +75,24 @@ static inline BOOL _Py_GetFileInformationByName( return GetFileInformationByName(FileName, FileInformationClass, FileInfoBuffer, FileInfoBufferSize); } +static inline BOOL _Py_GetFileInformationByName_ErrorIsTrustworthy(int error) +{ + switch(error) { + case ERROR_FILE_NOT_FOUND: + case ERROR_PATH_NOT_FOUND: + case ERROR_NOT_READY: + case ERROR_BAD_NET_NAME: + case ERROR_BAD_NETPATH: + case ERROR_BAD_PATHNAME: + case ERROR_INVALID_NAME: + case ERROR_FILENAME_EXCED_RANGE: + return TRUE; + case ERROR_NOT_SUPPORTED: + return FALSE; + } + return FALSE; +} + #endif #endif diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index cd3c67cc51ee63..dcb5e7a0e0408c 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4812,23 +4812,9 @@ os__path_isdir_impl(PyObject *module, PyObject *path) slow_path = FALSE; result = 0; } - } else { - switch(GetLastError()) { - case ERROR_FILE_NOT_FOUND: - case ERROR_PATH_NOT_FOUND: - case ERROR_NOT_READY: - case ERROR_BAD_NET_NAME: - case ERROR_BAD_NETPATH: - case ERROR_BAD_PATHNAME: - case ERROR_INVALID_NAME: - case ERROR_FILENAME_EXCED_RANGE: - /* These errors aren't worth retrying with the slow path */ + } else if (_Py_GetFileInformationByName_ErrorIsTrustworthy(GetLastError())) { slow_path = FALSE; result = 0; - case ERROR_NOT_SUPPORTED: - /* indicates the API couldn't be loaded */ - break; - } } } if (slow_path) { @@ -4923,23 +4909,9 @@ os__path_isfile_impl(PyObject *module, PyObject *path) slow_path = FALSE; result = 0; } - } else { - switch(GetLastError()) { - case ERROR_FILE_NOT_FOUND: - case ERROR_PATH_NOT_FOUND: - case ERROR_NOT_READY: - case ERROR_BAD_NET_NAME: - case ERROR_BAD_NETPATH: - case ERROR_BAD_PATHNAME: - case ERROR_INVALID_NAME: - case ERROR_FILENAME_EXCED_RANGE: - /* These errors aren't worth retrying with the slow path */ + } else if (_Py_GetFileInformationByName_ErrorIsTrustworthy(GetLastError())) { slow_path = FALSE; result = 0; - case ERROR_NOT_SUPPORTED: - /* indicates the API couldn't be loaded */ - break; - } } } if (slow_path) { @@ -5030,23 +5002,9 @@ os__path_exists_impl(PyObject *module, PyObject *path) slow_path = FALSE; result = 1; } - } else { - switch(GetLastError()) { - case ERROR_FILE_NOT_FOUND: - case ERROR_PATH_NOT_FOUND: - case ERROR_NOT_READY: - case ERROR_BAD_NET_NAME: - case ERROR_BAD_NETPATH: - case ERROR_BAD_PATHNAME: - case ERROR_INVALID_NAME: - case ERROR_FILENAME_EXCED_RANGE: - /* These errors aren't worth retrying with the slow path */ + } else if (_Py_GetFileInformationByName_ErrorIsTrustworthy(GetLastError())) { slow_path = FALSE; result = 0; - case ERROR_NOT_SUPPORTED: - /* indicates the API couldn't be loaded */ - break; - } } } if (slow_path) { @@ -5134,23 +5092,9 @@ os__path_islink_impl(PyObject *module, PyObject *path) else { result = 0; } - } else { - switch(GetLastError()) { - case ERROR_FILE_NOT_FOUND: - case ERROR_PATH_NOT_FOUND: - case ERROR_NOT_READY: - case ERROR_BAD_NET_NAME: - case ERROR_BAD_NETPATH: - case ERROR_BAD_PATHNAME: - case ERROR_INVALID_NAME: - case ERROR_FILENAME_EXCED_RANGE: - /* These errors aren't worth retrying with the slow path */ + } else if (_Py_GetFileInformationByName_ErrorIsTrustworthy(GetLastError())) { slow_path = FALSE; result = 0; - case ERROR_NOT_SUPPORTED: - /* indicates the API couldn't be loaded */ - break; - } } } if (slow_path) {