session: Add Session::free to avoid memory leak on drop#352
Open
pbar1 wants to merge 1 commit intoalexcrichton:masterfrom
Open
session: Add Session::free to avoid memory leak on drop#352pbar1 wants to merge 1 commit intoalexcrichton:masterfrom
pbar1 wants to merge 1 commit intoalexcrichton:masterfrom
Conversation
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. |
a667c1f to
dbdcc96
Compare
dbdcc96 to
514878a
Compare
yodaldevoid
reviewed
Jan 11, 2026
| /// 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> { |
Collaborator
There was a problem hiding this comment.
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.
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.
Mitigates #220 by adding a new function
Session::freewhich is a wrapper forlibssh2_session_free.Since
libssh2_session_freealso 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,
ssh2would leak sessions to the tune of about 1.5 MB total. Using this patch and callingSession::freeexplicitly reduces that to 0 MB when run under Valgrind.Notes:
Session::dropaside from guarding it with thefreedbool. 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.SessionInner.freedeither. There was some mention of that in the linked issue as well, requiring changingArc<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.