Skip to content

ext/standard: Fix use-after-free when a polled stream is closed - #23791

Open
nicolas-grekas wants to merge 1 commit into
php:masterfrom
nicolas-grekas:io-poll-closed-stream-uaf
Open

nicolas-grekas wants to merge 1 commit into
php:masterfrom
nicolas-grekas:io-poll-closed-stream-uaf

Conversation

@nicolas-grekas

Copy link
Copy Markdown
Contributor

Found while building an Io\Poll driver for revolt/event-loop.

StreamPollHandle caches the php_stream * it is constructed with, while the reference it takes is on the resource. fclose() frees the stream and leaves the resource alive as a closed one, so every later use of the handle reads freed memory:

[$r, $w] = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, 0);
$ctx = new Io\Poll\Context();
$watcher = $ctx->add(new StreamPollHandle($r), [Io\Poll\Event::Read]);
fclose($r);
$watcher->remove(); // segfault, and so do modifyEvents() and Context::add()

isValid() returns true there instead of false, and getStream() goes through the freed stream to reach the resource the handle already holds.

The stream is now resolved from the resource on each use, which gives NULL once it is closed. Context::add(), Watcher::modifyEvents() and Watcher::remove() already handle SOCK_ERR, so they throw InvalidHandleException and, for remove(), drop the watcher, the way they were written to.

StreamPollHandle cached the php_stream pointer it was constructed with, while the
reference it takes is on the resource. fclose() frees the stream and leaves the
resource alive as a closed one, so every later use of the handle read freed memory:
Context::add(), Watcher::modifyEvents() and Watcher::remove() crashed in
php_stream_cast(), and isValid() returned true.

The stream is now resolved from the resource on each use, which gives NULL once it
is closed. The call sites already handle SOCK_ERR, so they report the invalid handle
the way they were written to.
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.

1 participant