Skip to content

Commit

Permalink
gh-118422: Fix run_fileexflags() test (#118429)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
vstinner authored Apr 30, 2024
1 parent 587388f commit e93c39b
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 58 deletions.
3 changes: 3 additions & 0 deletions Include/internal/pycore_fileutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 6 additions & 7 deletions Modules/_testcapi/run.c
Original file line number Diff line number Diff line change
@@ -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>
Expand Down Expand Up @@ -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;
Expand Down
49 changes: 49 additions & 0 deletions Python/fileutils.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
55 changes: 4 additions & 51 deletions Python/pylifecycle.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit e93c39b

Please sign in to comment.