Skip to content

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jun 7, 2025

Use the PHP_STREAM_FLAG_NO_FCLOSE flag to prevent closing a stream while a handler is running. We already do this in some other places as well. Only handlers that do something with the stream afterwards need changes.

@nielsdos
Copy link
Member Author

nielsdos commented Jun 8, 2025

Test failures were unrelated and due to an issue at the time of branching on the master branch. Rebased for a CI run again now.

@nielsdos
Copy link
Member Author

nielsdos commented Sep 3, 2025

Rebased because the userspace code changed in between opening this PR and now.
@php/release-managers-85 are you fine merging this bugfix into master? It's not really a critical fix, it just protects the engine against code that misuses streams that would result in a memory safety issue.

@bukka
Copy link
Member

bukka commented Sep 3, 2025

You don't need to ping RM's for bug fix... Why is this master only? I can see the memory corruption and this seems like somthing that should not break real code if I'm not missing anything?

@nielsdos
Copy link
Member Author

nielsdos commented Sep 3, 2025

You don't need to ping RM's for bug fix... Why is this master only? I can see the memory corruption and this seems like somthing that should not break real code if I'm not missing anything?

We don't always apply those engine protections on lower branches if we can't fully gauge the consequences.
I don't think it will break real code, but you never really know what people do, I wanted to be extra careful. I can retarget to 8.3 though.

@bukka
Copy link
Member

bukka commented Sep 3, 2025

Yeah I guess there is a small chance that some code could relay on stream being closed in this way so probably master only is better.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Feel free to merge it. No need to wait for RM.

@nielsdos
Copy link
Member Author

nielsdos commented Sep 3, 2025

Yeah I guess there is a small chance that some code could relay on stream being closed in this way so probably master only is better.

If really necessary, it is always possible to backport in the future anyway once this code has sat into master long enough.

…r causes heap corruption

Use the PHP_STREAM_FLAG_NO_FCLOSE flag to prevent closing a stream while
a handler is running. We already do this in some other places as well.
Only handlers that do something with the stream afterwards need changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Closing a userspace stream inside a userspace handler causes heap corruption

3 participants