From 4cba722454901c4ca20ad7d8de72d2dd30b609f1 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 30 Apr 2024 15:40:24 +0200 Subject: [PATCH 1/3] gh-118422: Fix run_fileexflags() test Don't test undefined fileno() behavior on a closed file, but use fstat() as a reliable test if the file was closed or not. --- Modules/_testcapi/run.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/Modules/_testcapi/run.c b/Modules/_testcapi/run.c index 4fd98b82d762ff..61fb93358c57ce 100644 --- a/Modules/_testcapi/run.c +++ b/Modules/_testcapi/run.c @@ -71,21 +71,19 @@ run_fileexflags(PyObject *mod, PyObject *pos_args) PyErr_SetFromErrnoWithFilename(PyExc_OSError, filename); return NULL; } + int fd = fileno(fp); result = PyRun_FileExFlags(fp, filename, start, globals, locals, closeit, pflags); -#if defined(__linux__) || defined(MS_WINDOWS) || defined(__APPLE__) - /* The behavior of fileno() after fclose() is undefined, but it is - * the only practical way to check whether the file was closed. - * Only test this on the known platforms. */ - if (closeit && result && fileno(fp) >= 0) { + struct stat st; + if (closeit && result && fstat(fd, &st) == 0) { PyErr_SetString(PyExc_AssertionError, "File was not closed after excution"); Py_DECREF(result); fclose(fp); return NULL; } -#endif - if (!closeit && fileno(fp) < 0) { + + if (!closeit && fstat(fd, &st) != 0) { PyErr_SetString(PyExc_AssertionError, "Bad file descriptor after excution"); Py_XDECREF(result); return NULL; From fbef4730aac60b2f866281e251136763409b2f17 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 30 Apr 2024 16:32:20 +0200 Subject: [PATCH 2/3] Add _Py_IsValidFD() helper function --- Include/internal/pycore_fileutils.h | 3 ++ Modules/_testcapi/run.c | 7 ++-- Python/fileutils.c | 50 ++++++++++++++++++++++++++ Python/pylifecycle.c | 55 +++-------------------------- 4 files changed, 61 insertions(+), 54 deletions(-) diff --git a/Include/internal/pycore_fileutils.h b/Include/internal/pycore_fileutils.h index bc8100b58e8ea3..13f86b01bbfe8f 100644 --- a/Include/internal/pycore_fileutils.h +++ b/Include/internal/pycore_fileutils.h @@ -326,6 +326,9 @@ extern int _PyFile_Flush(PyObject *); extern int _Py_GetTicksPerSecond(long *ticks_per_second); #endif +// Export for '_testcapi' shared extension +PyAPI_FUNC(int) _Py_IsValidFD(int fd); + #ifdef __cplusplus } #endif diff --git a/Modules/_testcapi/run.c b/Modules/_testcapi/run.c index 61fb93358c57ce..21244d02967ebf 100644 --- a/Modules/_testcapi/run.c +++ b/Modules/_testcapi/run.c @@ -1,5 +1,7 @@ +#define PYTESTCAPI_NEED_INTERNAL_API #include "parts.h" #include "util.h" +#include "pycore_fileutils.h" // _Py_IsValidFD() #include #include @@ -75,15 +77,14 @@ run_fileexflags(PyObject *mod, PyObject *pos_args) result = PyRun_FileExFlags(fp, filename, start, globals, locals, closeit, pflags); - struct stat st; - if (closeit && result && fstat(fd, &st) == 0) { + if (closeit && result && _Py_IsValidFD(fd)) { PyErr_SetString(PyExc_AssertionError, "File was not closed after excution"); Py_DECREF(result); fclose(fp); return NULL; } - if (!closeit && fstat(fd, &st) != 0) { + if (!closeit && !_Py_IsValidFD(fd)) { PyErr_SetString(PyExc_AssertionError, "Bad file descriptor after excution"); Py_XDECREF(result); return NULL; diff --git a/Python/fileutils.c b/Python/fileutils.c index 54853ba2f75d9d..7bbf0190053c74 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -3050,3 +3050,53 @@ _Py_GetTicksPerSecond(long *ticks_per_second) return 0; } #endif + + +/* Check if a file descriptor is valid or not. + Return 0 if the file descriptor is invalid, return non-zero otherwise. */ +int +_Py_IsValidFD(int fd) +{ +/* dup() is faster than fstat(): fstat() can require input/output operations, + whereas dup() doesn't. There is a low risk of EMFILE/ENFILE at Python + startup. Problem: dup() doesn't check if the file descriptor is valid on + some platforms. + + fcntl(fd, F_GETFD) is even faster, because it only checks the process table. + It is preferred over dup() when available, since it cannot fail with the + "too many open files" error (EMFILE). + + bpo-30225: On macOS Tiger, when stdout is redirected to a pipe and the other + side of the pipe is closed, dup(1) succeed, whereas fstat(1, &st) fails with + EBADF. FreeBSD has similar issue (bpo-32849). + + Only use dup() on Linux where dup() is enough to detect invalid FD + (bpo-32849). +*/ + if (fd < 0) { + return 0; + } +#if defined(F_GETFD) && ( \ + defined(__linux__) || \ + defined(__APPLE__) || \ + defined(__wasm__)) + return fcntl(fd, F_GETFD) >= 0; +#elif defined(__linux__) + int fd2 = dup(fd); + if (fd2 >= 0) { + close(fd2); + } + return (fd2 >= 0); +#elif defined(MS_WINDOWS) + HANDLE hfile; + _Py_BEGIN_SUPPRESS_IPH + hfile = (HANDLE)_get_osfhandle(fd); + _Py_END_SUPPRESS_IPH + return (hfile != INVALID_HANDLE_VALUE + && GetFileType(hfile) != FILE_TYPE_UNKNOWN); +#else + struct stat st; + return (fstat(fd, &st) == 0); +#endif +} + diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 0f3ca4a6687514..48930150de5616 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2400,54 +2400,6 @@ init_import_site(void) return _PyStatus_OK(); } -/* Check if a file descriptor is valid or not. - Return 0 if the file descriptor is invalid, return non-zero otherwise. */ -static int -is_valid_fd(int fd) -{ -/* dup() is faster than fstat(): fstat() can require input/output operations, - whereas dup() doesn't. There is a low risk of EMFILE/ENFILE at Python - startup. Problem: dup() doesn't check if the file descriptor is valid on - some platforms. - - fcntl(fd, F_GETFD) is even faster, because it only checks the process table. - It is preferred over dup() when available, since it cannot fail with the - "too many open files" error (EMFILE). - - bpo-30225: On macOS Tiger, when stdout is redirected to a pipe and the other - side of the pipe is closed, dup(1) succeed, whereas fstat(1, &st) fails with - EBADF. FreeBSD has similar issue (bpo-32849). - - Only use dup() on Linux where dup() is enough to detect invalid FD - (bpo-32849). -*/ - if (fd < 0) { - return 0; - } -#if defined(F_GETFD) && ( \ - defined(__linux__) || \ - defined(__APPLE__) || \ - defined(__wasm__)) - return fcntl(fd, F_GETFD) >= 0; -#elif defined(__linux__) - int fd2 = dup(fd); - if (fd2 >= 0) { - close(fd2); - } - return (fd2 >= 0); -#elif defined(MS_WINDOWS) - HANDLE hfile; - _Py_BEGIN_SUPPRESS_IPH - hfile = (HANDLE)_get_osfhandle(fd); - _Py_END_SUPPRESS_IPH - return (hfile != INVALID_HANDLE_VALUE - && GetFileType(hfile) != FILE_TYPE_UNKNOWN); -#else - struct stat st; - return (fstat(fd, &st) == 0); -#endif -} - /* returns Py_None if the fd is not valid */ static PyObject* create_stdio(const PyConfig *config, PyObject* io, @@ -2461,8 +2413,9 @@ create_stdio(const PyConfig *config, PyObject* io, int buffering, isatty; const int buffered_stdio = config->buffered_stdio; - if (!is_valid_fd(fd)) + if (!_Py_IsValidFD(fd)) { Py_RETURN_NONE; + } /* stdin is always opened in buffered mode, first because it shouldn't make a difference in common use cases, second because TextIOWrapper @@ -2578,9 +2531,9 @@ create_stdio(const PyConfig *config, PyObject* io, Py_XDECREF(text); Py_XDECREF(raw); - if (PyErr_ExceptionMatches(PyExc_OSError) && !is_valid_fd(fd)) { + if (PyErr_ExceptionMatches(PyExc_OSError) && !_Py_IsValidFD(fd)) { /* Issue #24891: the file descriptor was closed after the first - is_valid_fd() check was called. Ignore the OSError and set the + _Py_IsValidFD() check was called. Ignore the OSError and set the stream to None. */ PyErr_Clear(); Py_RETURN_NONE; From a8ba0c1b08a0ae0058f1058a5175d1b35068cb92 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 30 Apr 2024 17:49:00 +0200 Subject: [PATCH 3/3] Fix WASI --- Python/fileutils.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/fileutils.c b/Python/fileutils.c index 7bbf0190053c74..e6a5391a3a28b5 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -3079,7 +3079,7 @@ _Py_IsValidFD(int fd) #if defined(F_GETFD) && ( \ defined(__linux__) || \ defined(__APPLE__) || \ - defined(__wasm__)) + (defined(__wasm__) && !defined(__wasi__))) return fcntl(fd, F_GETFD) >= 0; #elif defined(__linux__) int fd2 = dup(fd); @@ -3099,4 +3099,3 @@ _Py_IsValidFD(int fd) return (fstat(fd, &st) == 0); #endif } -