Skip to content

gh-135335: flush stdout/stderr in forkserver after preloading modules #135338

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jun 18, 2025

Conversation

duaneg
Copy link
Contributor

@duaneg duaneg commented Jun 10, 2025

If a preloaded module writes to stdout or stderr, and the stream is buffered, child processes will inherit the buffered data after forking. Attempt to prevent this by flushing the streams after preload.

…odules

If a preloaded module writes to stdout or stderr, and the stream is buffered,
child processes will inherit the buffered data after forking. Attempt to
prevent this by flushing the streams after preload.
@duaneg duaneg requested a review from efimov-mikhail June 16, 2025 09:06
…use a

not-completely-awful name for the module to test.
Copy link
Contributor

@efimov-mikhail efimov-mikhail left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I just have a last suggestion.

Copy link
Contributor

@efimov-mikhail efimov-mikhail left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner vstinner merged commit 9877d19 into python:main Jun 18, 2025
39 checks passed
@vstinner vstinner added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jun 18, 2025
@miss-islington-app
Copy link

Thanks @duaneg for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @duaneg for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 18, 2025
…odules (pythonGH-135338)

If a preloaded module writes to stdout or stderr, and the stream is buffered,
child processes will inherit the buffered data after forking. Attempt to
prevent this by flushing the streams after preload.
(cherry picked from commit 9877d19)

Co-authored-by: Duane Griffin <[email protected]>
Co-authored-by: Mikhail Efimov <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jun 18, 2025

GH-135670 is a backport of this pull request to the 3.14 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 18, 2025
…odules (pythonGH-135338)

If a preloaded module writes to stdout or stderr, and the stream is buffered,
child processes will inherit the buffered data after forking. Attempt to
prevent this by flushing the streams after preload.
(cherry picked from commit 9877d19)

Co-authored-by: Duane Griffin <[email protected]>
Co-authored-by: Mikhail Efimov <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Jun 18, 2025
@bedevere-app
Copy link

bedevere-app bot commented Jun 18, 2025

GH-135671 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 18, 2025
@vstinner
Copy link
Member

Thanks for the bug report and the fix @duaneg.

@duaneg
Copy link
Contributor Author

duaneg commented Jun 18, 2025

Thanks for the reviews, folks.

vstinner added a commit that referenced this pull request Jun 18, 2025
…modules (GH-135338) (#135671)

gh-135335: flush stdout/stderr in forkserver after preloading modules (GH-135338)

If a preloaded module writes to stdout or stderr, and the stream is buffered,
child processes will inherit the buffered data after forking. Attempt to
prevent this by flushing the streams after preload.
(cherry picked from commit 9877d19)

Co-authored-by: Duane Griffin <[email protected]>
Co-authored-by: Mikhail Efimov <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
vstinner added a commit that referenced this pull request Jun 18, 2025
…modules (GH-135338) (#135670)

gh-135335: flush stdout/stderr in forkserver after preloading modules (GH-135338)

If a preloaded module writes to stdout or stderr, and the stream is buffered,
child processes will inherit the buffered data after forking. Attempt to
prevent this by flushing the streams after preload.
(cherry picked from commit 9877d19)

Co-authored-by: Duane Griffin <[email protected]>
Co-authored-by: Mikhail Efimov <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
@efimov-mikhail
Copy link
Contributor

efimov-mikhail commented Jun 18, 2025

Thanks for thorough working on this PR!
Looking forward for the next one!

@duaneg
Copy link
Contributor Author

duaneg commented Jun 18, 2025

Thanks for thorough working on this PR! Looking forward for the next one!

Well, as it happens, I would love #132574 to get a review 🙂

It is a very simple bug and fix: an off-by-one error, that allows an out-of-bounds read, which affects basic string operations like replace(), and has been in our code base ~forever.

@efimov-mikhail
Copy link
Contributor

Ok, I'll take a look.

lkollar pushed a commit to lkollar/cpython that referenced this pull request Jun 19, 2025
…odules (python#135338)

If a preloaded module writes to stdout or stderr, and the stream is buffered,
child processes will inherit the buffered data after forking. Attempt to
prevent this by flushing the streams after preload.

Co-authored-by: Mikhail Efimov <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants