NotificationsYou must be signed in to change notification settings
Fork33.4k
Star69.8k

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

vstinner merged 11 commits intopython:mainfromcmaloney:cmaloney/fd_cleanup

Nov 1, 2024

Merged

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

vstinner merged 11 commits intopython:mainfromcmaloney:cmaloney/fd_cleanup

Nov 1, 2024

Conversation

Copy link

Contributor

cmaloney commentedOct 8, 2024•
edited by bedevere-appbot
Loading

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_atopenshould 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

cmaloney added2 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 thefd 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 theexisting memory.

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

d2cb365

bedevere-appbot added the awaiting review label

Oct 8, 2024

bedevere-appbot 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

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. */

Copy link

Member

vstinnerOct 9, 2024

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

cmaloney added2 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

Modules/_io/fileio.c

		#endif
		}

		PyMem_Free(self->stat_atopen);

Copy link

ContributorAuthor

cmaloneyOct 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 thatshould never be executed" (but that the compiler / tools that run currently won't validate...).

Copy link

Member

vstinnerOct 29, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

cmaloneyOct 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

cmaloney added2 commits

October 9, 2024 11:47

Remove assert accidentally commited

e2eebb0

_io: fix conditional

5299a04

vstinner reviewed

Oct 9, 2024

View reviewed changes

Copy link

Member

vstinner left a 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.

Copy link

ContributorAuthor

cmaloney commentedOct 9, 2024•
edited
Loading

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.

vstinner reviewed

Oct 10, 2024

View reviewed changes

Modules/_io/fileio.cShow resolvedHide resolved

cmaloneyand others added3 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

Copy link

ContributorAuthor

cmaloney commentedOct 30, 2024

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"

vstinner reviewed

Oct 30, 2024

View reviewed changes

Modules/_io/fileio.c Outdated

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

Copy link

Member

vstinnerOct 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

ContributorAuthor

cmaloneyOct 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 doesos.close() in these cases vs aninternal_close that does the OS close + other cleanup. Happy to drop though / definitely feels redundant to me in these two cases.

Modules/_io/fileio.c Outdated

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

Copy link

Member

vstinnerOct 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

ContributorAuthor

cmaloneyOct 31, 2024

Choose a reason for hiding this comment

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

Moderately, in theself->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

Modules/_io/fileio.c Outdated

		}

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

Copy link

Member

vstinnerOct 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 theif (stat_atopen != NULL) test.

Apply review fixes

e153d97

vstinner added the skip news label

Nov 1, 2024

vstinner merged commit72dd471 intopython:main

Nov 1, 2024

39 checks passed

bedevere-appbot removed the awaiting review label

Nov 1, 2024

Copy link

Member

vstinner commentedNov 1, 2024

Merged, thank you.

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 thefd 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 theexisting 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 thefd 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 theexisting memory.

Labels

skip news

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

cmaloney commentedOct 8, 2024• edited by bedevere-appbotLoading 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 commentedOct 9, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cmaloney commentedOct 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmaloneyOct 31, 2024• editedLoading 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!

Uh oh!

vstinner commentedNov 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cmaloney commentedOct 8, 2024•
edited by bedevere-appbot
Loading

cmaloney commentedOct 9, 2024•
edited
Loading

cmaloneyOct 31, 2024•
edited
Loading