Skip to content

Fix read buffer compaction in stream filter flush - #23439

Open
crystarm wants to merge 1 commit into
php:masterfrom
crystarm:fix/stream-filter-flush-buffer-compaction
Open

Fix read buffer compaction in stream filter flush#23439
crystarm wants to merge 1 commit into
php:masterfrom
crystarm:fix/stream-filter-flush-buffer-compaction

Conversation

@crystarm

Copy link
Copy Markdown
Contributor

php_stream_filter_flush() compacts unread data before appending buckets produced by a read filter.

The source and destination ranges may overlap, making the use of memcpy() undefined behavior. Additionally, readpos was reset before it was subtracted from writepos, so the buffer size was not adjusted and stale data could remain visible.

Use memmove() and adjust writepos before resetting readpos, matching the existing buffer compaction logic in php_stream_fill_read_buffer().

The issue was detected by static analysis: BUFFER_OVERLAP filter.c:[458:4].log

@crystarm
crystarm requested a review from bukka as a code owner August 24, 2026 15:23
@crystarm

Copy link
Copy Markdown
Contributor Author

Small clarification: the allocated buffer size (readbuflen) was not affected. Rather, resetting readpos before the subtraction left writepos unchanged, which could expose stale or duplicate bytes from the read buffer.

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

Could you add this test please?

--TEST--
stream_filter_remove() compacts unread data before appending flushed data
--FILE--
<?php
class ClosingSuffixFilter extends php_user_filter
{
    public function filter($in, $out, &$consumed, $closing): int
    {
        while ($bucket = stream_bucket_make_writeable($in)) {
            $consumed += $bucket->datalen;
            stream_bucket_append($out, $bucket);
        }
        if ($closing) {
            stream_bucket_append($out, stream_bucket_new($this->stream, 'END'));
        }
        return PSFS_PASS_ON;
    }
}
stream_filter_register('closing-suffix', ClosingSuffixFilter::class);
$stream = fopen('php://memory', 'w+');
fwrite($stream, 'abcdef');
rewind($stream);
$filter = stream_filter_append($stream, 'closing-suffix', STREAM_FILTER_READ);
var_dump(fread($stream, 2));
var_dump(stream_filter_remove($filter));
var_dump(stream_get_contents($stream));
?>
--EXPECT--
string(2) "ab"
bool(true)
string(7) "cdefEND"

Also, could you please rebase to 8.4 instead of master?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants