Skip to content

ext/standard: limit maximum number of filter chains - #22110

Merged
LamentXU123 merged 3 commits into
php:masterfrom
Sjord:limit_filters
Aug 2, 2026
Merged

ext/standard: limit maximum number of filter chains#22110
LamentXU123 merged 3 commits into
php:masterfrom
Sjord:limit_filters

Conversation

@Sjord

@Sjord Sjord commented May 21, 2026

Copy link
Copy Markdown
Contributor

@Sjord
Sjord requested a review from bukka as a code owner May 21, 2026 13:01
@Sjord

Sjord commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

It's looking pretty good for the RFC, so it would be nice to get a review of this PR.

@Sjord

Sjord commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

RFC is accepted. @bukka could you review this?

@bukka

bukka commented Jun 26, 2026

Copy link
Copy Markdown
Member

Will check this or next weekend hopefully.

@Sjord

Sjord commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@bukka Could you review this please?

@Sjord

Sjord commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@LamentXU123 , @arnaud-lb Could one of you perhaps review this pull request?

@LamentXU123 LamentXU123 self-assigned this Jul 16, 2026
LamentXU123
LamentXU123 reviewed Jul 16, 2026
View reviewed changes

@LamentXU123 LamentXU123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the late response and thank you for your work.

I am not an expert in this topic so just my 2 cents

Comment thread ext/standard/php_fopen_wrapper.c Outdated
Comment thread ext/standard/php_fopen_wrapper.c Outdated
@LamentXU123

LamentXU123 commented Jul 16, 2026

Copy link
Copy Markdown
Member

@Sjord Thanks. Now please wait for others reviews on this topic because i am not an expert on this.Also keep my reviews unresolved because I want to hear from others opinions. This looks good to me :)

@LamentXU123
LamentXU123 requested a review from Girgias July 16, 2026 13:28
arnaud-lb
arnaud-lb reviewed Jul 16, 2026
View reviewed changes
Comment thread ext/standard/tests/filters/max_filter_chain.phpt
Comment thread ext/standard/php_fopen_wrapper.c Outdated
Comment thread ext/standard/php_fopen_wrapper.c Outdated
TimWolla
TimWolla requested changes Jul 16, 2026
View reviewed changes

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Didn't actually look at the code, but these stood out. There might be more cases where you're using spaces instead of tabs.

Comment thread ext/standard/php_fopen_wrapper.c Outdated
Comment thread main/streams/filter.c Outdated
@Sjord
Sjord requested a review from arnaud-lb July 16, 2026 18:58
arnaud-lb
arnaud-lb approved these changes Jul 17, 2026
View reviewed changes

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me otherwise!

Comment thread main/streams/php_stream_filter_api.h Outdated
Comment thread ext/standard/php_fopen_wrapper.c Outdated
LamentXU123
LamentXU123 approved these changes Jul 18, 2026
View reviewed changes

@LamentXU123 LamentXU123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awaiting Jakub for final review 👍

@LamentXU123 LamentXU123 assigned bukka and unassigned LamentXU123 Jul 19, 2026
@Sjord

Sjord commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

@bukka Could you review/merge this?

@Sjord
Sjord requested a review from TimWolla July 27, 2026 09:28
@Sjord Sjord changed the title Limit maximum number of filter chains ext/standard: limit maximum number of filter chains Jul 30, 2026
bukka
bukka approved these changes Aug 2, 2026
View reviewed changes

@bukka bukka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@bukka

bukka commented Aug 2, 2026

Copy link
Copy Markdown
Member

It would be good if you could push NEWS and UPGRADING update now so we can just squash it and merge it through GH...

@LamentXU123

LamentXU123 commented Aug 2, 2026

Copy link
Copy Markdown
Member

Leave the remaining work to me I will do it when merging. Thanks.

@Sjord @jvoisin @LamentXU123
Limit php://filter chains
1e91e5b 
Limit php://filter URLs to 16 filters by default and allow applications to
override the limit with filter.max_filter_count.

Co-Authored-By: jvoisin <julien.voisin@dustri.org>
@LamentXU123

LamentXU123 commented Aug 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Note: Since the PR to remove the value of parameter in errors of file functions is merged. The test file need to be slightly changed.

@LamentXU123
LamentXU123 merged commit ed34ad4 into php:master Aug 2, 2026
17 of 18 checks passed
@Sjord

Sjord commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TimWolla TimWolla TimWolla requested changes
@bukka bukka bukka approved these changes
@arnaud-lb arnaud-lb arnaud-lb approved these changes
@LamentXU123 LamentXU123 LamentXU123 approved these changes

Assignees

No one assigned

Labels

ABI break Extension: standard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Sjord @bukka @LamentXU123 @TimWolla @arnaud-lb