Fix memory leak when user filter does not fully consume the input brigade buckets by ndossche · Pull Request #20058 · php/php-src

ndossche · 2025-10-04T10:53:31Z

Since the _php_stream_write_filtered() function assumes that the input brigade will be emptied (as it clears one of the bucket brigades), any unconsumed bucket will leak. The other filters do not suffer from this as they abort cleanly with an error code.
It's also possible to fix this in _php_stream_write_filtered() with an extra check, but since the user filter is the only one that has this bug and already checks for this condition, we fix it there instead.

For completeness, the alternative i.e. a fix in _php_stream_write_filtered() would look like this:

diff --git a/main/streams/streams.c b/main/streams/streams.c
index 372ed6635c3..720f3c15dd7 100644
--- a/main/streams/streams.c
+++ b/main/streams/streams.c
@@ -1242,6 +1242,15 @@ static ssize_t _php_stream_write_filtered(php_stream *stream, const char *buf, s
 		if (status != PSFS_PASS_ON) {
 			break;
 		}
+		/* If the filter did not process the entire input brigade, then the buckets need to be freed
+		 * manually or they will be lost when setting up the brigades for next iteration. */
+		if (UNEXPECTED(brig_inp->head)) {
+			do {
+				bucket = brig_inp->head;
+				php_stream_bucket_unlink(bucket);
+				php_stream_bucket_delref(bucket);
+			} while (brig_inp->head);
+		}
 		/* brig_out becomes brig_in.
 		 * brig_in will always be empty here, as the filter MUST attach any un-consumed buckets
 		 * to its own brigade */

arnaud-lb

This looks good to me

It's also possible to fix this in _php_stream_write_filtered() with an extra check, but since the user filter is the only one that has this bug and already checks for this condition, we fix it there instead.

Could we add an assertion in _php_stream_write_filtered() so we don't make this error when implementing new internal filters?

bukka · 2025-10-29T09:49:56Z

I will take a look in couple of weeks.

ndossche · 2025-11-15T21:24:53Z

Note to self: would need to remove the XFAIL of ext/standard/tests/filters/stream_filter_register_class_coerce_consumed_by_ref_param.phpt after the merge of #20045

bukka

I think this is right place to do it because user filter breaks the contract (input brigade must be empty after processing) but I'm not sure about this change ff84cb0 that applies the same logic for PSFS_FEED_ME and PSFS_FEED_FATAL. I think it should be dropped there and applied only to user filter. I'm not sure if even the out part is needed anywhere else than user filter..?

ndossche · 2025-11-29T17:06:00Z

@bukka Okay I agree. I've moved that logic to the user filter now.

ndossche · 2025-11-29T17:22:17Z

 	}

+	/* Filter could've broken contract and added buckets anyway. */
+	if (ret == PSFS_FEED_ME && buckets_out->head) {


I think I'd need to double check whether this ret check is too specific (e.g. do we also need to do anything on FATAL).

No, I just checked this and it seems fine with other return values.

iliaal · 2026-07-21T14:26:38Z

Ran into this same bucket family while looking at GH-22845. Two observations on the current patch.

The userfilter_filter drain also covers the append-time run in php_stream_filter_append_ex, since that path calls the same dispatcher, so brig_in there is drained too. A per-caller fix would have missed it.

The buckets_out drain is gated on ret == PSFS_FEED_ME. The caller switches have no default case, so a filter that returns an out-of-range status with output buckets still attached keeps them linked to the stack brigade (dangling bucket->brigade). Widening the check to ret != PSFS_PASS_ON, or clamping an unknown status to PSFS_ERR_FATAL in the dispatcher, would close that.

matthiasgoergens · 2026-09-23T06:38:49Z

@ndossche, iliaal found a case on my duplicate of this fix (#23267) that also applies to this branch: the input-brigade drain in userfilter_filter() runs for every status, but _php_stream_fill_read_buffer() keeps brig_in across calls, so a filter that returns PSFS_FEED_ME after putting its buckets back on $in loses them. His reproducer, a filter that defers twice on a three-chunk file, returns 8192 bytes on this branch; on PHP-8.3 at this branch's base it returns 24576 (with the leak warnings this PR fixes). Both measured on debug builds.

I stacked one commit on top of your 270ec55 at https://github.com/matthiasgoergens/php-src/tree/gh20058-feed-me-gating: the input drain gated on ret != PSFS_FEED_ME, the output drain widened to ret != PSFS_PASS_ON as iliaal suggested above, and the deferral case as a phpt. With it the ext/standard/tests/filters directory passes on a debug build, your user_filter_no_consume_leak.phpt included; without it the new test fails. Cherry-pick it or take whichever part you want; I will close #23267 in favour of this one either way.

iliaal · 2026-09-23T10:29:17Z

The PSFS_FEED_ME gate brings the leak back: only the read loop re-presents $in, while _php_stream_write_filtered(), php_stream_filter_append_ex() and _php_stream_filter_flush() discard their brigades on that status, and the read loop drops them on exit. So the callers have to free what they abandon; with that on top of a7a81df the leak probes are clean and the deferral case still returns 24576: iliaal@dfbc43af312

ndossche · 2026-09-23T20:38:16Z

I don't quite follow. @iliaal what do you like me to do here? Feel free to take this over.

iliaal · 2026-09-23T21:18:04Z

Sorry, that was unclear. Your drain fixes the leak but loses data in one case, and Matthias's fix for that case brings the leak back in other callers.

A user filter can put buckets back on $in and return PSFS_FEED_ME to get them again with the next chunk. _php_stream_fill_read_buffer() keeps brig_in across iterations, so on PHP-8.3 a filter that defers twice on a three-chunk file reads 24576 bytes (plus the leak warnings). With the drain on every status it reads 8192.

a7a81df skips the drain on PSFS_FEED_ME, which fixes reads, but _php_stream_write_filtered(), php_stream_filter_append_ex() and _php_stream_filter_flush() drop their brigades on that status, and the read loop drops them when it exits, so those input buckets leak again. dfbc43a frees them in each of those places.

Perhaps cherry-picking those 2 commits from https://github.com/iliaal/php-src/tree/gh20058-feed-me-callers is easiest (if approach/idea makes sense).

…gade buckets Since the _php_stream_write_filtered() function assumes that the input brigade will be emptied (as it clears one of the bucket brigades), any unconsumed bucket will leak. The other filters do not suffer from this as they abort cleanly with an error code. It's also possible to fix this in _php_stream_write_filtered() with an extra check, but since the user filter is the only one that has this bug and already checks for this condition, we fix it there instead. For completeness, a fix in _php_stream_write_filtered() would look like this: ```diff diff --git a/main/streams/streams.c b/main/streams/streams.c index 372ed66..720f3c15dd7 100644 --- a/main/streams/streams.c +++ b/main/streams/streams.c @@ -1242,6 +1242,15 @@ static ssize_t _php_stream_write_filtered(php_stream *stream, const char *buf, s if (status != PSFS_PASS_ON) { break; } + /* If the filter did not process the entire input brigade, then the buckets need to be freed + * manually or they will be lost when setting up the brigades for next iteration. */ + if (UNEXPECTED(brig_inp->head)) { + do { + bucket = brig_inp->head; + php_stream_bucket_unlink(bucket); + php_stream_bucket_delref(bucket); + } while (brig_inp->head); + } /* brig_out becomes brig_in. * brig_in will always be empty here, as the filter MUST attach any un-consumed buckets * to its own brigade */ ``` Co-authored-by: Gina Peter Banyard <girgias@php.net>

…or any non-PASS_ON status _php_stream_fill_read_buffer() keeps the input brigade across calls, so a filter that returns PSFS_FEED_ME may put buckets back on $in to see them again with the next chunk. Draining the input brigade for every status frees those buckets and truncates the stream: a filter that defers twice on a three-chunk file returns 8192 bytes instead of 24576, as iliaal found while reviewing php#23267. Drain the input brigade only when the filter did not return PSFS_FEED_ME. Also drain leftover output buckets for any status other than PSFS_PASS_ON, so an out-of-range status cannot leave them linked to the stack brigade.

userfilter_filter() keeps the input brigade on PSFS_FEED_ME so the read loop can re-present it, but _php_stream_write_filtered(), php_stream_filter_append_ex() and _php_stream_filter_flush() discard their stack brigades on that status, and _php_stream_fill_read_buffer() drops them when its loop exits. Free what is left in each of those places.

ndossche · 2026-09-24T20:58:38Z

@iliaal You're correct.
I cherry-picked and rebased.
I also think that the repeated while loop pattern of the last commit may be moved to a common helper, the repetition is ugly ;p

Girgias · 2026-09-26T17:19:58Z

+				while ((bucket = brig_in.head)) {
+					php_stream_bucket_unlink(bucket);
+					php_stream_bucket_delref(bucket);
+				}


Is it possible to move this logic (that is repeated quite a few times) into a function?

ndossche requested a review from bukka as a code owner October 4, 2025 10:53

github-actions Bot added the Extension: standard label Oct 4, 2025

ndossche mentioned this pull request Oct 4, 2025

Refactor and add tests for stream filters #20045

Merged

ndossche requested a review from arnaud-lb October 28, 2025 21:46

arnaud-lb approved these changes Oct 29, 2025

View reviewed changes

bukka reviewed Nov 23, 2025

View reviewed changes

ndossche commented Nov 29, 2025

View reviewed changes

ndossche requested a review from bukka December 14, 2025 21:40

NickSdot mentioned this pull request Aug 24, 2026

ext/standard: Adds and modifies XFAIL tests for filter bucket leaks #23440

Open

iliaal mentioned this pull request Aug 31, 2026

Fix leaks from unprocessed user filter buckets #23267

Closed

ndossche and others added 4 commits September 24, 2026 22:27

Move broken contract logic

df88b7e

ndossche force-pushed the user-filter-leak-1 branch from 270ec55 to df4e0f3 Compare September 24, 2026 20:57

ndossche requested review from Girgias, SakiTakamachi, adoy, derickr, devnexen, dstogov and kocsismate as code owners September 24, 2026 20:57

ndossche requested a review from TimWolla as a code owner September 24, 2026 20:57

ndossche changed the base branch from PHP-8.3 to PHP-8.4 September 24, 2026 20:57

TimWolla removed their request for review September 25, 2026 07:04

ndossche removed request for Girgias, SakiTakamachi, adoy, derickr, devnexen, dstogov and kocsismate September 25, 2026 08:24

Fix extra leak

4526f72

Girgias reviewed Sep 26, 2026

View reviewed changes

Conversation

ndossche commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnaud-lb left a comment

Choose a reason for hiding this comment

Uh oh!

bukka commented Oct 29, 2025

Uh oh!

ndossche commented Nov 15, 2025

Uh oh!

bukka left a comment

Choose a reason for hiding this comment

Uh oh!

ndossche commented Nov 29, 2025

Uh oh!

ndossche Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

ndossche Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

iliaal commented Jul 21, 2026

Uh oh!

matthiasgoergens commented Sep 23, 2026

Uh oh!

iliaal commented Sep 23, 2026

Uh oh!

ndossche commented Sep 23, 2026

Uh oh!

iliaal commented Sep 23, 2026

Uh oh!

ndossche commented Sep 24, 2026

Uh oh!

Girgias Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ndossche commented Oct 4, 2025 •

edited

Loading