Skip to content

gh-120754: Ensure _stat_atopen is cleared on fd change #125166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vstinner merged 11 commits into from
Nov 1, 2024
Merged

gh-120754: Ensure _stat_atopen is cleared on fd change #125166

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d57abd0
gh-120754: _io Ensure stat cache is cleared on fd change
cmaloney Oct 8, 2024
d2cb365
gh-120754: _pyio ensure _stat_atopen is cleared on fd change
cmaloney Oct 8, 2024
854ae91
_io: Add curlies since already modifying if
cmaloney Oct 9, 2024
c6da86e
_io: Always clear self->stat_atopen
cmaloney Oct 9, 2024
0f64483
pyio: Always clear stat_atopen just before close
cmaloney Oct 9, 2024
e2eebb0
Remove assert accidentally commited
cmaloney Oct 9, 2024
5299a04
_io: fix conditional
cmaloney Oct 9, 2024
d749709
Clear before internal_close to match _pyio os.close callsites
cmaloney Oct 22, 2024
b429587
Move to conditional free, simplify code, drop comment
cmaloney Oct 30, 2024
f08defa
Merge branch 'main' into cmaloney/fd_cleanup
cmaloney Oct 30, 2024
e153d97
Apply review fixes
cmaloney Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Lib/_pyio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,6 +1480,7 @@ def __init__(self, file, mode='r', closefd=True, opener=None):
"""
if self._fd >= 0:
# Have to close the existing file first.
self._stat_atopen = None
try:
if self._closefd:
os.close(self._fd)
Expand Down Expand Up @@ -1583,6 +1584,7 @@ def __init__(self, file, mode='r', closefd=True, opener=None):
if e.errno != errno.ESPIPE:
raise
except:
self._stat_atopen = None
if owned_fd is not None:
os.close(owned_fd)
raise
Expand Down Expand Up @@ -1756,6 +1758,7 @@ def close(self):
called more than once without error.
"""
if not self.closed:
self._stat_atopen = None
try:
if self._closefd:
os.close(self._fd)
Expand Down
19 changes: 13 additions & 6 deletions Modules/_io/fileio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ internal_close(fileio *self)
_Py_END_SUPPRESS_IPH
Py_END_ALLOW_THREADS
}
PyMem_Free(self->stat_atopen);
self->stat_atopen = NULL;
if (err < 0) {
errno = save_errno;
PyErr_SetFromErrno(PyExc_OSError);
Expand Down Expand Up @@ -178,6 +180,8 @@ _io_FileIO_close_impl(fileio *self, PyTypeObject *cls)
PyErr_Clear();
}
}
PyMem_Free(self->stat_atopen);
self->stat_atopen = NULL;
Copy link
Copy Markdown
Member

@vstinner vstinner Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal_close() already does the same, so it's redundant, no?

Copy link
Copy Markdown
Contributor Author

@cmaloney cmaloney Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tradeoff for matching what _pyio does which does os.close() in these cases vs an internal_close that does the OS close + other cleanup. Happy to drop though / definitely feels redundant to me in these two cases.

rc = internal_close(self);
if (res == NULL) {
_PyErr_ChainExceptions1(exc);
Expand Down Expand Up @@ -266,10 +270,13 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode,
assert(PyFileIO_Check(state, self));
#endif
if (self->fd >= 0) {
PyMem_Free(self->stat_atopen);
self->stat_atopen = NULL;
Copy link
Copy Markdown
Member

@vstinner vstinner Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal_close() already does the same, so it's redundant, no?

Copy link
Copy Markdown
Contributor Author

@cmaloney cmaloney Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moderately, in the self->closefd is false case behavior would differ from _pyio, but that will be caught by the PyMem_Free + PyMem_New below, so think this is always redundant / not needed

if (self->closefd) {
/* Have to close the existing file first. */
if (internal_close(self) < 0)
if (internal_close(self) < 0) {
return -1;
}
}
else
self->fd = -1;
Expand Down Expand Up @@ -455,7 +462,9 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode,
#endif
}

PyMem_Free(self->stat_atopen);
Copy link
Copy Markdown
Contributor Author

@cmaloney cmaloney Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure which is preferred here. The Free + New back to back feels weird, but version with the conditional feels like "extra work/code that should never be executed" (but that the compiler / tools that run currently won't validate...).

Copy link
Copy Markdown
Member

@vstinner vstinner Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if (ptr != NULL) free(ptr) would be more regular and avoid the 4 lines long comment.

Copy link
Copy Markdown
Contributor Author

@cmaloney cmaloney Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to that, dropped comment and simplified / back to always calling PyMem_New

if (self->stat_atopen != NULL) {
Copy link
Copy Markdown
Member

@vstinner vstinner Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyMem_Free(NULL) is well defined: it does nothing, so you might omit the if (stat_atopen != NULL) test.

PyMem_Free(self->stat_atopen);
}
self->stat_atopen = PyMem_New(struct _Py_stat_struct, 1);
if (self->stat_atopen == NULL) {
PyErr_NoMemory();
Expand Down Expand Up @@ -523,10 +532,8 @@ _io_FileIO___init___impl(fileio *self, PyObject *nameobj, const char *mode,
internal_close(self);
_PyErr_ChainExceptions1(exc);
}
if (self->stat_atopen != NULL) {
PyMem_Free(self->stat_atopen);
self->stat_atopen = NULL;
}
PyMem_Free(self->stat_atopen);
self->stat_atopen = NULL;
Comment thread
vstinner marked this conversation as resolved.

done:
#ifdef MS_WINDOWS
Expand Down