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

SSH: Fix incorrect timeout failures on disconnects. #1479

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@JDepooter
Contributor

JDepooter commented May 10, 2017

libcurl is leaking memory when using sftp and a timeout. This was first reported on the mailing list.

The memory leak reported on the mailing list is a symptom of a larger problem - SSH sessions are not freed when a timeout is used. I suspect this problem is also present on other platforms, but may not manifest itself as a memory leak on all platforms.

I used the Visual Studio memory analysis tools to find that libssh2 is leaking some WinCNG structs for each connection. This is because libcurl is never calling the libssh2_session_free function when a timeout is set. Without a timeout, the function is called and things get cleaned up correctly.

In a disconnect situation, the close_all_connections function replaces the easy handle associated with a connection with a 'closure handle'. This will reset all progress values associated with the connection to zero values. As a result, calculations using these values will return incorrect values.

The close_all_connections function eventually calls into the sftp_disconnect function, which runs the ssh_block_statemach function. In this function, if a timeout is set, the "time left" value will always be negative, because the start time has been reset to zero. This means that if a timeout is set, disconnections always fail with a timeout error, rather than finish successfully. Since the easy handle has been replaced with the closure handle, this timeout error will not be reported, regardless of the verbose setting used on the original easy handle.

SSH: Only check for timeouts with valid progress values
In a disconnect situation, the easy handle associated with a connection may have been replaced with a 'closure handle'. This will reset all progress values associated with the connection to zero values. As a result, calculations using these values will return incorrect values.

In particular, if a timeout is set, the 'time left' value will always be negative, because the start time has been reset to zero. This means that if a timeout is set, disconnections always fail with a timeout error, rather than finish successfully.
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 10, 2017

Member

Two style warnings, as below, but I would like to see this problem fixed slightly differently. I think we should make Curl_timeleft return 0 if it there is no legitimate timeout that can trigger. But I think perhaps the problem should be taken yet another step backwards: this happens because there's no start time set so there can be no timeout there. Maybe we should instead make sure that we set a start time even for the closure handle so it can timeout in the case where the connection is stalled or something like that?

./ssh.c:2838:2: warning: Trailing whitespace (TRAILINGSPACE)
./ssh.c:2842:2: warning: Trailing whitespace (TRAILINGSPACE)
Member

bagder commented May 10, 2017

Two style warnings, as below, but I would like to see this problem fixed slightly differently. I think we should make Curl_timeleft return 0 if it there is no legitimate timeout that can trigger. But I think perhaps the problem should be taken yet another step backwards: this happens because there's no start time set so there can be no timeout there. Maybe we should instead make sure that we set a start time even for the closure handle so it can timeout in the case where the connection is stalled or something like that?

./ssh.c:2838:2: warning: Trailing whitespace (TRAILINGSPACE)
./ssh.c:2842:2: warning: Trailing whitespace (TRAILINGSPACE)
@JDepooter

This comment has been minimized.

Show comment
Hide comment
@JDepooter

JDepooter May 10, 2017

Contributor

I suspected this change wouldn't get merged unscathed ;)

I think we should make Curl_timeleft return 0 if it there is no legitimate timeout that can trigger.

I did consider this, but not know too much about the internals of libcurl, I wanted to make as minimal a change as possible. Ditto for changing the closure handle. Good to know that these are legitimate options. I will update this pull request in the next day or two.

Contributor

JDepooter commented May 10, 2017

I suspected this change wouldn't get merged unscathed ;)

I think we should make Curl_timeleft return 0 if it there is no legitimate timeout that can trigger.

I did consider this, but not know too much about the internals of libcurl, I wanted to make as minimal a change as possible. Ditto for changing the closure handle. Good to know that these are legitimate options. I will update this pull request in the next day or two.

bagder added a commit that referenced this pull request May 18, 2017

ssh: ignore timeouts during disconnect
... as otherwise it risks not cleaning up the libssh2 handle properly
which leads to memory leak!

This is an alternative approch to what's done in #1479

Bug: https://curl.haxx.se/mail/lib-2017-04/0024.html
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 18, 2017

Member

I just submitted my alternative fix proposal as a separate PR: #1495

Member

bagder commented May 18, 2017

I just submitted my alternative fix proposal as a separate PR: #1495

@bagder bagder closed this in f31760e May 20, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018

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