Use-after-free in stream_filter_remove() called from a stream callback

[EdmondDantes]

stream_filter_remove() flushes the filter, then unlinks and frees it. The flush can run PHP code: a user filter's filter() method, or a user wrapper's stream_write() for the flushed data. If that code removes the same filter, the outer call continues on freed memory. A user filter can also remove itself (or another filter of its stream) from filter(), or from other code while filter() is suspended in a Fiber; userfilter_filter() and the filter chain walks then use the freed filter after the callback returns. Valgrind shows invalid reads in userfilter_filter() and php_stream_filter_remove().

The fix:

• userfilter_filter() sets a new stream flag, PHP_STREAM_FLAG_USER_FILTER_RUNNING, around the callback, next to the existing PHP_STREAM_FLAG_NO_FCLOSE. stream_filter_remove() refuses with a warning while it is set, as fclose() already does.
• After the flush, stream_filter_remove() checks that the filter resource is still alive, for the case where a user wrapper removed it from stream_write().

Tests: ext/standard/tests/filters/stream_filter_remove_in_filter.phpt (removal from filter() and while filter() is suspended in a Fiber) and stream_filter_remove_during_flush.phpt (removal from a user wrapper's stream_write() during the flush). Both fail without the fix, with invalid reads under valgrind.

Known limits, left for separate changes: the flag is saved and restored per call, so Fibers resumed out of order can clear it early, as with PHP_STREAM_FLAG_NO_FCLOSE today; pclose() and closedir() free a stream without checking PHP_STREAM_FLAG_NO_FCLOSE.

[devnexen]

Hi @EdmondDantes, can the following test be added please ?

--TEST--
stream_filter_remove() while user filters of the same stream are suspended in several Fibers
--FILE--
<?php
class Suspender extends php_user_filter {
    public function filter($in, $out, &$consumed, bool $closing): int {
        while ($bucket = stream_bucket_make_writeable($in)) {
            $consumed += $bucket->datalen;
            stream_bucket_append($out, $bucket);
        }

        if (Fiber::getCurrent()) {
            Fiber::suspend();
        }

        return PSFS_PASS_ON;
    }
}

stream_filter_register('suspender', 'Suspender');

$stream = fopen('php://memory', 'w+');
$filter = stream_filter_append($stream, 'suspender', STREAM_FILTER_WRITE);

$fiber1 = new Fiber(fn() => fwrite($stream, 'one'));
$fiber2 = new Fiber(fn() => fwrite($stream, 'two'));
$fiber1->start();
$fiber2->start();
$fiber1->resume();
var_dump($fiber1->getReturn());

var_dump(stream_filter_remove($filter));

$fiber2->resume();
var_dump($fiber2->getReturn());

var_dump(stream_filter_remove($filter));

rewind($stream);
var_dump(stream_get_contents($stream));
?>
--EXPECTF--
int(3)

Warning: stream_filter_remove(): Unable to remove filter while a user filter of the same stream is running in %s on line %d
bool(false)
int(3)
bool(true)
string(6) "onetwo"

[EdmondDantes]

@devnexen
Oh... this isn’t the kind of issue that can be fixed easily...
And what should we do? Add a counter to the stream structure?

[EdmondDantes]

The requested test is now added, currently with XFAIL. On this PR head, the first Fiber clears the running flag while the second is still suspended. Removal then succeeds prematurely; Valgrind with USE_ZEND_ALLOC=0 confirms use-after-free when the second Fiber resumes.

A possible follow-up is a per-stream counter, rather than saving/restoring a bit. Short sketch (not implemented or tested yet):

/* New field in php_stream, shared by both filter chains. */
uint32_t active_user_filters;

/* userfilter_filter(): replace the save/set of both flags,
 * before property access and the callback. */
stream->active_user_filters++;

/* On BOTH exits after entering the protected region:
 * after cleanup, including the property-update exception path. */
ZEND_ASSERT(stream->active_user_filters > 0);
stream->active_user_filters--;

/* stream_filter_remove(): replace the running-flag check. */
if (filter->chain && filter->chain->stream
        && filter->chain->stream->active_user_filters > 0) {
    php_error_docref(NULL, E_WARNING,
        "Unable to remove filter while a user filter of the same stream is running");
    RETURN_FALSE;
}

/* fclose(): retain the independent NO_FCLOSE protection. */
if ((stream->flags & PHP_STREAM_FLAG_NO_FCLOSE)
        || stream->active_user_filters > 0) {
    /* Existing warning and RETURN_FALSE. */
}

This removes all save/set/restore of NO_FCLOSE and USER_FILTER_RUNNING from userfilter_filter(); retaining the old NO_FCLOSE restoration would still leave its Fiber-order bug. Stream allocation already zeroes the structure. Cleanup remains protected until the decrement.

The counter handles nested callbacks and either Fiber completion order, but changes to the public php_stream structure require ABI review for PHP 8.4. It does not address pclose()/closedir() or make overlapping operations on a stream generally safe. Before replacing XFAIL with a passing regression, we should also check both completion orders, absence of stuck close protection, sibling/read-chain removal, and exception cleanup.

[devnexen]

The counter is not a bad idea, might be tough for a stable branch however to update the struct. Another alternative would be, I think, the check should be per filter rather than per stream. The only unsafe case is removing the filter that is itself inside its callback.

[EdmondDantes]

The counter is not a bad idea, might be tough for a stable branch however to update the struct. Another alternative would be, I think, the check should be per filter rather than per stream. The only unsafe case is removing the filter that is itself inside its callback.

I decided to go with a counter.

[devnexen]

It seems a working solution, but I prefer to defer the final decision on @bukka for this one.