Skip to content

session dealloc #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
eliwe opened this issue Jan 10, 2019 · 1 comment
Closed

session dealloc #55

eliwe opened this issue Jan 10, 2019 · 1 comment

Comments

@eliwe
Copy link
Contributor

eliwe commented Jan 10, 2019

Hi,

I have an issue that originates from the usage of dealloc to close and release sessions.
The code in dealloc calls
c_ssh2.libssh2_session_disconnect(self._session, b"end") c_ssh2.libssh2_session_free(self._session)

In session disconnect a message is sent to the other side singling the disconnect. But by the time this function is called (dealloc is called at some point after the object is not used any more) the underlaying socket might have been closed and libssh2 will return and error . Worst yet is that the fileno gets reused for another connection and the message will be sent to a different peer (this happened to me).

I changed the API for session to include a release function. I also removed the free from channel as it shouldn't be called if the session has been release.
see:
https://github.com/eliwe/ssh2-python

I didn't send a pull request as its not a proper fix (it does solve my immediate problem).

I didn't write sample code to recreate the issue as its not 100 reproducible.

@pkittenis
Copy link
Member

Hi there,

Thanks for the interest and report.

Interesting, yes, had not considered that the file descriptor may have been reused by the time the session object is deallocated. Disconnecting returning an error is ok, hence why return code is not checked, and that was really only done in case session.disconnect was not called. But since it can cause an issue, it should be removed.

If you want to make a PR with the disconnect line removed I'm happy to review.

Channel free should not be removed, however. That's a memory leak. Channel is guaranteed to be deallocated safely, before session, since it holds a reference to the session object it was spawned from. See open_session code. It would be obvious if it was being freed after session - that would cause a segfault.

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

No branches or pull requests

2 participants