Fix Io\Poll memory-safety and error-handling issues#22316
Conversation
| /* New FD, create new event */ | ||
| ZEND_ASSERT(unique_events < max_events); | ||
| if (unique_events >= max_events) { | ||
| continue; |
There was a problem hiding this comment.
it seems continue also skips the oneshot bookkeeping below it, so any oneshot fd dropped past the cap leaves the backend's tracking desynced. Gate only the buffer write on unique_events < max_events and still fall through to the tracking.
There was a problem hiding this comment.
Right, the continue also skipped the oneshot tracking. Gated only the buffer write now, so the bookkeeping runs regardless of the cap.
| php_poll_set_current_errno_error(ctx); | ||
| return -1; | ||
| } | ||
| if (nfds == 0) { |
There was a problem hiding this comment.
nit: I guess you can just merge as if (...) else if (...) here wdyt?
Several memory-safety and error-handling issues in the new Io\Poll API, found by review and confirmed under valgrind: - Watcher kept a raw pointer to its Context's php_poll_ctx with no reference, so dropping the Context while holding a Watcher left remove()/modify() dereferencing freed memory (use-after-free). The Context now neutralizes its watchers (active=false, poll_ctx=NULL) before it is destroyed, so those calls throw InactiveWatcherException. - StreamPollHandle took a reference on the stream resource in the constructor but never released it, leaking the descriptor for the rest of the request. Store the zend_resource and release it in the handle cleanup; the php_stream may already be freed by then (e.g. the user closed it), so the cleanup must not dereference it. - Watcher and Context had no get_gc handler, so reference cycles through Watcher::$data were uncollectable. Add get_gc for both. - Context, Watcher and StreamPollHandle were cloneable through the default handler, which shallow-copied the backing php_poll_ctx and the watcher map by pointer and double-freed them on destruction. Mark all three uncloneable. - Calling __construct() a second time on a Context or StreamPollHandle replaced the backing context or handle data without releasing the first, leaking it. Throw if the object is already constructed. - php_poll_set_error() dereferenced its ctx argument, but the add(), modify(), remove() and wait() entry points call it precisely when ctx is NULL. Guard the dereference. - The epoll and poll backends returned -1 from wait() without recording the error, so the thrown exception carried a stale code. Set it from errno, which maps EINTR to the interrupted error. - The kqueue grouped-event path bounded the result buffer only with a ZEND_ASSERT, allowing an out-of-bounds write in release builds when more distinct descriptors were ready than the caller's maxEvents. Cap the buffer write at runtime while still running the oneshot bookkeeping so the backend's tracking stays in sync. - php_poll_create_by_name() dereferenced a NULL backend name; guard it.
iliaal
force-pushed
the
fix/io-poll-memory-safety
branch
from
ef34a6e to
71ddf7d
Compare
|
@ndossche fixes pushed: clone now throws (Context/Watcher/StreamPollHandle marked uncloneable), double Couldn't reproduce the |
See Slack, I already fixed that yesterday ;) I'll have a review look at this PR this evening. |
3 participants
Fixes memory-safety and error-handling bugs in the new Io\Poll API, found by review and confirmed under valgrind.
Tests cover the clone guard, the double-construct guard, the watcher-outlives-context UAF, the fd release, and the get_gc cycle.