Fix SSH2 resource ownership and stream error handling#6113
Open
binaryfire wants to merge 6 commits into
Open
Conversation
Initialize the session and SFTP pointers on directory stream handles before the stream is exposed. This gives directory reads and cleanup the same complete handle state as ordinary SFTP file streams. Add a PHPT that creates a remote directory, enumerates its entries through the SFTP wrapper, and removes the fixture.
Register direct wrapper sessions before authentication so every failure path releases an initialized Zend resource. Register the SFTP subsystem against that owned session and transfer the existing references to successful wrapper streams. Release temporary SFTP resources after one-shot stat and mutation operations. Add PHPT coverage for authentication failure, direct subsystem registration, and repeated wrapper operations.
Introduce a shared parent-session predicate and apply it across channel, listener, public-key, and SFTP operations. Child resources now return their normal PHP failure values after the owning session is closed while destructors still release Zend references and local bookkeeping. Correct the fetched-stream refcount decrement and cover the resource graph with a regression that disconnects parent sessions before exercising and closing their children.
Initialize channels as blocking PHP streams and let explicit nonblocking operations expose libssh2 backpressure without entering the coroutine-yielding hook. Preserve negative transport errors, derive EOF only from zero-byte reads, and publish channel-local stream metadata. Pass per-stream timeouts directly to socket polling so sub-second precision is retained and one channel cannot alter the shared session socket for later operations. Cover blocking defaults, nonblocking EAGAIN, stream errors, EOF transitions, timeout metadata, and timeout isolation.
Keep negative SFTP file reads as errors instead of marking them as EOF. For directory streams, make each native result own the EOF state so clean completion, transport failure, and a closed parent session remain distinguishable through PHP's stream API. Add regressions for normal file and directory completion as well as failed reads after the parent SSH session is disconnected.
Limit the descriptor-leak regression to one warmup failure and one observed failure. This remains sufficient to detect the leaked session descriptor while staying below OpenSSH 9.8+'s default per-source penalty floor. Document the limit so later test expansion does not temporarily block unrelated tests sharing the SSH server.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The bundled SSH2 extension has one connected set of resource-lifetime and stream-contract bugs:
These paths share the same ownership graph and stream implementations. Splitting them across pull requests would leave unsafe intermediate states.
Change
The series contains five implementation commits plus one test-isolation follow-up:
Channel timeouts are passed directly to the owning socket poll. They no longer mutate shared session socket state, so a timeout on one channel cannot affect another channel or a later SFTP operation on the same SSH session.
Tests
New PHPT coverage exercises:
Each implementation commit was built and tested at its own boundary. The complete added SSH2 test set also passes against the final extension build.
Review notes
The affected ownership paths were compared with PECL ssh2 1.5.0. This keeps the established Zend resource model where it applies, while retaining Swoole's coroutine-aware I/O and adding the Swoole-specific channel timeout and nonblocking behavior.
The parent-guard commit also changes
(data->refcount)--to(*(data->refcount))--in the allocation-failure cleanup forssh2_fetch_stream(). That branch cannot be forced through a normal PHPT without a production test hook, but the current expression advances the pointer instead of decrementing the referenced count.