Fix GH-22844: StreamPollHandle use-after-free after fclose()

[iliaal]

StreamPollHandle stored the raw php_stream * but referenced only the resource container, so fclose() freed the stream while the handle kept a dangling pointer. isValid(), getStream() and the internal get_fd (reached via Context::add()) then dereferenced freed memory. The accessors now re-derive the stream from the held resource and report a closed resource as absent: getStream() returns null, isValid() returns false, and the stub return becomes resource|null.

A watcher also outlived the stream it polled: once the stream closed its backend registration could no longer be removed by fd, so wait() could dereference the freed watcher. On epoll the interest even survives fclose() when the fd is duplicated, so removing it lazily is not enough. Watchers are now unregistered from php_stream_free(), while the fd is still valid, via a poll_watchers back-reference on the stream. Reproduced under ASAN.

Fixes #22844

[arnaud-lb]

We should not cache the fd as it becomes unrelated to the original stream after fclose(). At this point fd may be invalid (best case) or may point to another file, so calling php_poll_remove() may remove an unrelated watch:

use Io\Poll\{Context, Event};

$context = new Context();

list($r0, $w0) = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, 0);

$watcher0 = $context->add(new StreamPollHandle($r0), [Event::Read]);

fclose($r0);

// Likely reuses $r0's fd number
list($r1, $w1) = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, 0);

$watcher1 = $context->add(new StreamPollHandle($r1), [Event::Read]);

// Likely removes r1
$watcher0->remove();

fwrite($w1, 'hello');

// Should return [$watcher1], but will likely return []
var_dump($context->wait(0));

But if we don't remove fd, we may run into epoll's edge cases with duplicated fds (I don't know about other backends).

We should probably add a callback from php_stream_free to notify Io\Poll when we close a stream, so that Io\Poll gets an opportunity to remove a fd just before it's closed. I did that in arnaud-lb@ecc3309 to support weak Io\Poll handles.

[iliaal]

You're right, dropped the fd cache and added the php_stream_free callback you suggested: a watched stream unregisters its watchers from every context at close time, while the fd is still valid, so there's no stale registration for a reused fd to hit. This also covers the case the fd cache couldn't, where a duplicated fd keeps the epoll interest alive past fclose(). StreamPollHandle isn't a singleton like your weak handle, so the back-reference is a per-stream watcher set (stream->poll_watchers) plus a cached fd per watcher. One caveat it shares with #32: if something closes the fd before php_stream_free (e.g. bzopen() on a watched socket), the close-time removal can't fire. If you'd rather own this in #32, I'll rebase onto it.

[arnaud-lb]

One caveat it shares with #32: if something closes the fd before php_stream_free (e.g. bzopen() on a watched socket), the close-time removal can't fire

Good point. User stream wrappers would be an issue as well: not only the stream returned by stream_cast() may be closed later, but it can also return a completely different stream on subsequent calls.

We need to handle these otherwise the fix is incomplete.

For bzopen() and similar cases, we could generalize the php_stream_free callback so that the bz stream can use it to be notified when the inner stream is closed, and in turn notify its watcher or handle.

For user stream wrappers, maybe StreamPollHandle should call stream_cast() once and store the result during instantiation.

This is starting to get complicated, but it may be worth it for ergonomics, if that's doable.

An alternative may be to not do anything (besides fixing the UAF) and just document the behavior. Users would be recommended to remove watchers before closing the related stream, otherwise phantom events can be triggered.

Another alternative is that StreamPollHandle dup() the fd, so that it can not be closed by someone else, but I don't think that's a good solution.

Maybe wait for @bukka's input before spending too much time on this.

[bukka]

I tried to just map it internally in fdtable but that just had some limitations and his callback will be needed for weakref so it's actually a good approach. We should not null the stream and there were few more issues so the full fix in #23822 .

I think that bzopen() and similar should never close it unless they duplicate it. I looked to bz2 and it actually had a double close bug (first through BZ2_bzclose and then doing php_stream_free which closes it too) without doing any dup. Fix in #23824 . I also checked zlib and it uses dup so it should be fine.