GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-120754: Ensure _stat_atopen is cleared on fd change by cmaloney · Pull Request #125166 · python/cpython

Conversation

cmaloney commented Oct 8, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

While adding _stat_atopen I accidentally created a leak in the C implementation because the _stat_atopen member was set to NULL in the C implementation, but the memory wasn't freed. That was resolved, but it showed there were a number of potential cases where _stat_atopen should be cleared (the information is out of date because the fd changed) that it was not. In this PR I audited by reading through FileIO (in _io and _pyio), and in all cases where the fd is changed/closed made sure to clear/reset _stat_atopen as well.

Followup from: #124225 (comment), cc: @vstinner

Issue: Speed up open().read() pattern by reducing the number of system calls #120754

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cmaloney added 2 commits

October 8, 2024 16:41


          pythongh-120754: _io Ensure stat cache is cleared on fd change

d57abd0

Performed an audit of `fileio.c` and `_pyio` and made sure anytime the
fd changes the stat result, if set, is also cleared/changed.

There's one case where it's not cleared, if code would clear it in
__init__, keep the memory allocated and just do another fstat with the
existing memory.


          pythongh-120754: _pyio ensure _stat_atopen is cleared on fd change

d2cb365

bedevere-app Bot added the awaiting review label

Oct 8, 2024

bedevere-app Bot mentioned this pull request

Oct 8, 2024

Speed up open().read() pattern by reducing the number of system calls #120754

Closed


          _io: Add curlies since already modifying if

854ae91

vstinner reviewed

Oct 9, 2024

View reviewed changes

Comment thread

Modules/_io/fileio.c Outdated

		if (internal_close(self) < 0)
		/* Have to close the existing file first.

		This leaves the stat so we can reuse the memory. */

vstinner Oct 9, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think that this optimization is worth it, I would prefer to always clear the cache in close().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cmaloney reacted with thumbs up emoji

All reactions

👍 1 reaction

cmaloney added 2 commits

October 9, 2024 11:42


          _io: Always clear self->stat_atopen

c6da86e


          pyio: Always clear stat_atopen just before close

0f64483

cmaloney commented

Oct 9, 2024

View reviewed changes

Comment thread

Modules/_io/fileio.c

		#endif
		}

		PyMem_Free(self->stat_atopen);

cmaloney Oct 9, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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...).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Oct 29, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cmaloney Oct 30, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cmaloney added 2 commits

October 9, 2024 11:47


          Remove assert accidentally commited

e2eebb0


          _io: fix conditional

5299a04

vstinner reviewed

Oct 9, 2024

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There are compiler warnings that you can see in the review tab.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cmaloney reacted with eyes emoji

All reactions

👀 1 reaction

cmaloney commented Oct 9, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

I think the latest commit fixes the compiler warnings (was a testing/debugging assert I accidentally committed with a missing ;, latest changes remove that assert). Not able to find them in the latest changes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reviewed

Oct 10, 2024

View reviewed changes

Comment thread

Modules/_io/fileio.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

cmaloney and others added 3 commits

October 22, 2024 14:40


          Clear before internal_close to match _pyio os.close callsites

d749709


          Move to conditional free, simplify code, drop comment

b429587


          Merge branch 'main' into cmaloney/fd_cleanup

f08defa

cmaloney commented Oct 30, 2024

Copy link

Copy Markdown

Contributor Author

CI "Tests / Ubuntu SSL tests with OpenSSL" failed because of what looks like a network hang / not these changes: Received 164773440 of 168967744 (97.5%), 0.0 MBs/sec while doing "Configure ccache action"

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reviewed

Oct 30, 2024

View reviewed changes

Comment thread

Modules/_io/fileio.c Outdated

		}
		}
		PyMem_Free(self->stat_atopen);
		self->stat_atopen = NULL;

vstinner Oct 30, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cmaloney Oct 31, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_io/fileio.c Outdated

		#endif
		if (self->fd >= 0) {
		PyMem_Free(self->stat_atopen);
		self->stat_atopen = NULL;

vstinner Oct 30, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cmaloney Oct 31, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_io/fileio.c Outdated

		}

		PyMem_Free(self->stat_atopen);
		if (self->stat_atopen != NULL) {

vstinner Oct 30, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cmaloney reacted with thumbs up emoji

All reactions

👍 1 reaction


          Apply review fixes

e153d97

vstinner added the skip news label

Nov 1, 2024

vstinner merged commit 72dd471 into python:main

Nov 1, 2024

bedevere-app Bot removed the awaiting review label

Nov 1, 2024

vstinner commented Nov 1, 2024

Copy link

Copy Markdown

Member

Merged, thank you.

cmaloney reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cmaloney deleted the cmaloney/fd_cleanup branch

November 1, 2024 21:55

picnixz pushed a commit to picnixz/cpython that referenced this pull request

Dec 8, 2024


          pythongh-120754: _io Ensure stat cache is cleared on fd change (pytho…

ed81044

…n#125166)

Performed an audit of `fileio.c` and `_pyio` and made sure anytime the
fd changes the stat result, if set, is also cleared/changed.

There's one case where it's not cleared, if code would clear it in
__init__, keep the memory allocated and just do another fstat with the
existing memory.

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request

Jan 12, 2025


          pythongh-120754: _io Ensure stat cache is cleared on fd change (pytho…

167302c

…n#125166)

Performed an audit of `fileio.c` and `_pyio` and made sure anytime the
fd changes the stat result, if set, is also cleared/changed.

There's one case where it's not cleared, if code would clear it in
__init__, keep the memory allocated and just do another fstat with the
existing memory.

scasella mentioned this pull request

Sep 26, 2026

Free-threading: concurrent FileIO close/truncate can double-free stat_atopen #158196

Open

Reviewers

vstinner

vstinner left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

cmaloney commented Oct 8, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

cmaloney commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cmaloney commented Oct 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmaloney Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 1, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cmaloney commented Oct 8, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

cmaloney commented Oct 9, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

cmaloney Oct 31, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.