From fde06a7aa4ef0f96582867acb2524e44a6400b18 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 4 Oct 2025 12:50:49 +0200 Subject: [PATCH 1/2] Fix memory leak when user filter does not fully consume the input brigade 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 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 */ ``` Co-authored-by: Gina Peter Banyard --- .../filters/user_filter_no_consume_leak.phpt | 22 +++++++++++++++++++ ext/standard/user_filters.c | 6 +++++ 2 files changed, 28 insertions(+) create mode 100644 ext/standard/tests/filters/user_filter_no_consume_leak.phpt diff --git a/ext/standard/tests/filters/user_filter_no_consume_leak.phpt b/ext/standard/tests/filters/user_filter_no_consume_leak.phpt new file mode 100644 index 0000000000000..c13b69c5892cd --- /dev/null +++ b/ext/standard/tests/filters/user_filter_no_consume_leak.phpt @@ -0,0 +1,22 @@ +--TEST-- +User filter that does not consume any input bucket leaks +--FILE-- + +--EXPECTF-- +Warning: fwrite(): Unprocessed filter buckets remaining on input brigade in %s on line %d +int(0) diff --git a/ext/standard/user_filters.c b/ext/standard/user_filters.c index 962eaaba7d6b0..3335a7c2a1fec 100644 --- a/ext/standard/user_filters.c +++ b/ext/standard/user_filters.c @@ -190,6 +190,12 @@ php_stream_filter_status_t userfilter_filter( } if (buckets_in->head) { + php_stream_bucket *bucket; + do { + bucket = buckets_in->head; + php_stream_bucket_unlink(bucket); + php_stream_bucket_delref(bucket); + } while (buckets_in->head); php_error_docref(NULL, E_WARNING, "Unprocessed filter buckets remaining on input brigade"); } From 270ec55da8aa0a6444564878f3309ddbfe851d7b Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+ndossche@users.noreply.github.com> Date: Sat, 29 Nov 2025 18:05:41 +0100 Subject: [PATCH 2/2] Move broken contract logic --- ext/standard/user_filters.c | 10 ++++++++++ main/streams/filter.c | 7 ------- main/streams/streams.c | 14 +------------- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/ext/standard/user_filters.c b/ext/standard/user_filters.c index 3335a7c2a1fec..0028441df7ffb 100644 --- a/ext/standard/user_filters.c +++ b/ext/standard/user_filters.c @@ -199,6 +199,16 @@ php_stream_filter_status_t userfilter_filter( php_error_docref(NULL, E_WARNING, "Unprocessed filter buckets remaining on input brigade"); } + /* Filter could've broken contract and added buckets anyway. */ + if (ret == PSFS_FEED_ME && buckets_out->head) { + php_stream_bucket *bucket; + do { + bucket = buckets_out->head; + php_stream_bucket_unlink(bucket); + php_stream_bucket_delref(bucket); + } while (buckets_out->head); + } + /* filter resources are cleaned up by the stream destructor, * keeping a reference to the stream resource here would prevent it * from being destroyed properly */ diff --git a/main/streams/filter.c b/main/streams/filter.c index 1ce03d41513cd..f6e47405dedec 100644 --- a/main/streams/filter.c +++ b/main/streams/filter.c @@ -355,13 +355,6 @@ PHPAPI int php_stream_filter_append_ex(php_stream_filter_chain *chain, php_strea Reset stream's internal read buffer since the filter is "holding" it. */ stream->readpos = 0; stream->writepos = 0; - - /* Filter could have added buckets anyway, but signalled that it did not return any. Discard them. */ - while (brig_out.head) { - bucket = brig_out.head; - php_stream_bucket_unlink(bucket); - php_stream_bucket_delref(bucket); - } break; case PSFS_PASS_ON: /* If any data is consumed, we cannot rely upon the existing read buffer, diff --git a/main/streams/streams.c b/main/streams/streams.c index 372ed6635c3f0..6628ec76c3fe2 100644 --- a/main/streams/streams.c +++ b/main/streams/streams.c @@ -631,12 +631,6 @@ PHPAPI zend_result _php_stream_fill_read_buffer(php_stream *stream, size_t size) /* when a filter needs feeding, there is no brig_out to deal with. * we simply continue the loop; if the caller needs more data, * we will read again, otherwise out job is done here */ - - /* Filter could have added buckets anyway, but signalled that it did not return any. Discard them. */ - while ((bucket = brig_outp->head)) { - php_stream_bucket_unlink(bucket); - php_stream_bucket_delref(bucket); - } break; case PSFS_ERR_FATAL: @@ -1275,16 +1269,10 @@ static ssize_t _php_stream_write_filtered(php_stream *stream, const char *buf, s /* some fatal error. Theoretically, the stream is borked, so all * further writes should fail. */ consumed = (ssize_t) -1; - ZEND_FALLTHROUGH; + break; case PSFS_FEED_ME: /* need more data before we can push data through to the stream */ - /* Filter could have added buckets anyway, but signalled that it did not return any. Discard them. */ - while (brig_inp->head) { - bucket = brig_inp->head; - php_stream_bucket_unlink(bucket); - php_stream_bucket_delref(bucket); - } break; }