Fix memory leak when user filter does not fully consume the input brigade buckets

[ndossche]

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]

I will take a look in couple of weeks.

[ndossche]

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]

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

[ndossche]

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

[ndossche]

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

[iliaal]

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.