Skip to content

session: Add Session::free to avoid memory leak on drop#352

Open
pbar1 wants to merge 1 commit intoalexcrichton:masterfrom
pbar1:0.9.4-sessionleakfix
Open

session: Add Session::free to avoid memory leak on drop#352
pbar1 wants to merge 1 commit intoalexcrichton:masterfrom
pbar1:0.9.4-sessionleakfix

Conversation

@pbar1
Copy link

@pbar1 pbar1 commented Jan 6, 2026

Mitigates #220 by adding a new function Session::free which is a wrapper for libssh2_session_free.

Since libssh2_session_free also sometimes performs IO (and thus return EAGAIN when in nonblocking mode), the thinking here is to provide an escape hatch for nonblocking mode to explicitly shutdown/free the session.

Prior to this patch, when used within the context of a (closed source, unfortunately) bulk SSH connection tool connecting to 30k hosts in nonblocking mode, ssh2 would leak sessions to the tune of about 1.5 MB total. Using this patch and calling Session::free explicitly reduces that to 0 MB when run under Valgrind.

Notes:

  • Does not change the behavior of Session::drop aside from guarding it with the freed bool. There was an example in the linked issue of taking the TCP stream and dropping it manually, but this does not do that. Should make for a more backwards compatible change.
  • Does not guard each method with a check for SessionInner.freed either. There was some mention of that in the linked issue as well, requiring changing Arc<Mutex<SessionInner>> -> Arc<Mutex<Option<SessionInner>>>. If that's desired, I can make that change on top of this one too. It would be more correct/sound, but a more invasive change.

@yodaldevoid
Copy link
Collaborator

Thanks for the PR. I'll take a closer look at this tomorrow.

Just a nit, but please re-add the trailing newline that was removed in your patch.

A quick look at CI says I need to do a pass on tests, but I don't think that has anything to do with your changes.

@pbar1 pbar1 force-pushed the 0.9.4-sessionleakfix branch from a667c1f to dbdcc96 Compare January 7, 2026 07:36
@pbar1 pbar1 force-pushed the 0.9.4-sessionleakfix branch from dbdcc96 to 514878a Compare January 7, 2026 07:50
/// this function explicitly instead of relying on `drop` - otherwise it
/// would result in a memory leak if the session fails to free due to
/// blocking.
pub fn free(&self) -> Result<(), Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make more sense for this method to be pub fn free(self) -> Result<(), (Self, Error)>. This would ensure the session is not re-used after it is freed when it is freed successfully.

Session::drop wouldn't need to be modified if the Session was std::mem::forgeted when successfully freed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants