https://github.com/python/cpython/commit/e93c39b47ea623dfaf61f80775ad4747b16... commit: e93c39b47ea623dfaf61f80775ad4747b163efe5 branch: main author: Victor Stinner <vstinner@python.org> committer: vstinner <vstinner@python.org> date: 2024-04-30T22:32:55+02:00 summary: gh-118422: Fix run_fileexflags() test (#118429) Don't test the undefined behavior of fileno() on a closed file, but use fstat() as a reliable test if the file was closed or not. files: M Include/internal/pycore_fileutils.h M Modules/_testcapi/run.c M Python/fileutils.c M Python/pylifecycle.c 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 4fd98b82d762ff..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 <stdio.h> #include <errno.h> @@ -71,21 +73,18 @@ 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) { + if (closeit && result && _Py_IsValidFD(fd)) { 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 && !_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..e6a5391a3a28b5 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -3050,3 +3050,52 @@ _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__) && !defined(__wasi__))) + 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 a672d8c7915ccd..790398e944f58e 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2410,54 +2410,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, @@ -2471,8 +2423,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 @@ -2588,9 +2541,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;