rust-lang / rust-lang/ssh2-rs

Leaking LIBSSH2_SESSION objects

Open
#220 10 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
566
Forks
163
Avg merge
4h 51m
Merged PRs (30d)
2

Description

I'm using ssh2-rs in an async project (via async-ssh2), and I have stumbled across a problem with the way sessions are freed.

The Drop impl for SessionInner just calls libssh2_session_free and disregards the return value. This is a problem in both blocking and nonblocking modes:

  • In non-blocking mode, libssh2_session_free is likely to return LIBSSH2_ERROR_EAGAIN and need to be called again when the socket is ready
  • In blocking mode, if the caller has set a timeout then it may return LIBSSH2_ERROR_TIMEOUT (and in fact if the caller has not set a timeout, it may hang indefinitely if there is a network problem).

In both cases, if you don't repeatedly call it until you get a 0 returned, then the resources are not released.

In my experience so far, in case of network disturbance it can be pretty difficult to get libsshb2 to properly release the session. The only thing that has worked for me so far is this:

impl Drop for SessionInner {
    fn drop(&mut self) {
        self.set_blocking(true);
        loop {
            let rc = unsafe { raw::libssh2_session_free(self.raw) };
            if rc == 0 {
                break;
            } else {
                // Timeout - drop the transport,
                // which will hopefully cause the socket to close so we can try again
                if let Some(tcp) = self.tcp.take() {
                    let tcp = unsafe { std::net::TcpStream::from_raw_fd(tcp.as_raw_fd()) };
                    let _ = tcp.shutdown(std::net::Shutdown::Both);
                }
            }
        }
    }
}

So, if there is a timeout when calling libssh2_session_free, we shut down the socket and try again. The next time around libssh2 realizes the socket is shut down and finishes the cleanup.

This biggest problem with this is that since tcp is Box<dyn AsRawFd>, we can't call shutdown on it directly, all we can do is get the FD from it, and then create a new TcpStream around it - which is not sound, because then there are two TcpStreams with the same FD. In my testing this has not caused an issue yet, but the documentation explicitly states not to do that. Also, theoretically the the FD might be associated with some other stream type that implements AsRawFd, (eg. UnixStream).

The above is also not good from an async point of view, because the drop fn blocks the thread for potentially a long time. To make it work better in an async context some more work could be required, but that probably should be for another issue.

Finally - whatever approach is used here needs to be replicated for Windows also.

I'm happy to make a PR, if someone can suggest how to avoid the unsoundness described above.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the Drop implementation for SessionInner and trace its calls to libssh2_session_free in blocking and nonblocking modes. Compare the available transport abstractions, including the Windows path, and determine how cleanup can complete without duplicate stream ownership or an indefinitely blocking drop. Done means session resources are released safely across supported platforms and modes.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.