Skip to content

Fix leak of stream buckets left behind by misbehaving user filters - #24602

Open
zeynallow wants to merge 1 commit into
php:PHP-8.4from
zeynallow:fix-bucket-leak
Open

zeynallow wants to merge 1 commit into
php:PHP-8.4from
zeynallow:fix-bucket-leak

Conversation

@zeynallow

Copy link
Copy Markdown

When a stream filter does not move the buckets of its input brigade (for example a
user filter that simply returns PSFS_PASS_ON without calling
stream_bucket_make_writeable()), php_stream_write_filtered() and
php_stream_fill_read_buffer() swap the brigades and then memset() the old input
brigade to zero. The unconsumed buckets are still linked to it, so they are leaked.
user_filters.c already warns about this situation
("Unprocessed filter buckets remaining on input brigade"), but nothing frees the buckets.

Separately, when a write filter returns PSFS_FEED_ME (or PSFS_ERR_FATAL) after
appending buckets to the output brigade, php_stream_write_filtered() only cleans up
the input brigade, so the output buckets are leaked. The read path already discards
the output brigade in this case; the write path now does the same.

Reproducer (debug build):

class f extends php_user_filter {
    public function filter($in, $out, &$consumed, bool $closing): int {
        return PSFS_PASS_ON;
    }
}
stream_filter_register("f", "f");
$fp = fopen("php://memory", "w+");
stream_filter_append($fp, "f", STREAM_FILTER_WRITE);
fwrite($fp, "Hello");
// main/streams/filter.c(72) : Freeing ... (48 bytes)
// === Total 1 memory leaks detected ===

Changes:

  • Add a small php_stream_bucket_brigade_discard() helper in main/streams/streams.c.
  • Discard leftover input buckets before swapping brigades, in both the read and write
    filter chains (the data was already reported as unprocessed and was being dropped
    anyway, so behaviour is unchanged apart from the leak).
  • On PSFS_FEED_ME / PSFS_ERR_FATAL in the write path, also discard the output brigade.
  • New test ext/standard/tests/filters/user_filter_unconsumed_buckets_leak.phpt
    covering the read/PASS_ON, write/PASS_ON and write/FEED_ME cases.
  • When merged up to PHP-8.6 / master, the --XFAIL-- This leaks memory sections of
    stream_filter_register_mock_class_filter.phpt and
    stream_filter_register_class_coerce_consumed_by_ref_param.phpt can be removed;
    both pass with this fix.

Tested with a debug build: ext/standard/tests/filters and ext/standard/tests/file
all pass on PHP-8.4; on master the two former XFAIL tests now pass.

When a filter returns PSFS_PASS_ON without consuming its input brigade,
the read and write filter chains reset that brigade with memset() and
leak the unconsumed buckets. When a write filter returns PSFS_FEED_ME or
PSFS_ERR_FATAL after appending output buckets, those were leaked too.

Discard leftover buckets in both cases.

@blaze27444 blaze27444 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Files changed

@blaze27444

Copy link
Copy Markdown

When a stream filter does not move the buckets of its input brigade (for example a user filter that simply returns PSFS_PASS_ON without calling stream_bucket_make_writeable()), php_stream_write_filtered() and php_stream_fill_read_buffer() swap the brigades and then memset() the old input brigade to zero. The unconsumed buckets are still linked to it, so they are leaked. user_filters.c already warns about this situation ("Unprocessed filter buckets remaining on input brigade"), but nothing frees the buckets.

Separately, when a write filter returns PSFS_FEED_ME (or PSFS_ERR_FATAL) after appending buckets to the output brigade, php_stream_write_filtered() only cleans up the input brigade, so the output buckets are leaked. The read path already discards the output brigade in this case; the write path now does the same.

Reproducer (debug build):

class f extends php_user_filter {
    public function filter($in, $out, &$consumed, bool $closing): int {
        return PSFS_PASS_ON;
    }
}
stream_filter_register("f", "f");
$fp = fopen("php://memory", "w+");
stream_filter_append($fp, "f", STREAM_FILTER_WRITE);
fwrite($fp, "Hello");
// main/streams/filter.c(72) : Freeing ... (48 bytes)
// === Total 1 memory leaks detected ===

Changes:

  • Add a small php_stream_bucket_brigade_discard() helper in main/streams/streams.c.
  • Discard leftover input buckets before swapping brigades, in both the read and write
    filter chains (the data was already reported as unprocessed and was being dropped
    anyway, so behaviour is unchanged apart from the leak).
  • On PSFS_FEED_ME / PSFS_ERR_FATAL in the write path, also discard the output brigade.
  • New test ext/standard/tests/filters/user_filter_unconsumed_buckets_leak.phpt
    covering the read/PASS_ON, write/PASS_ON and write/FEED_ME cases.
  • When merged up to PHP-8.6 / master, the --XFAIL-- This leaks memory sections of
    stream_filter_register_mock_class_filter.phpt and
    stream_filter_register_class_coerce_consumed_by_ref_param.phpt can be removed;
    both pass with this fix.

Tested with a debug build: ext/standard/tests/filters and ext/standard/tests/file all pass on PHP-8.4; on master the two former XFAIL tests now pass.

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.

2 participants