SSH: Fix incorrect timeout failures on disconnects. #1479
Conversation
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.
|
Two style warnings, as below, but I would like to see this problem fixed slightly differently. I think we should make |
|
I suspected this change wouldn't get merged unscathed ;)
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. |
... 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
|
I just submitted my alternative fix proposal as a separate PR: #1495 |
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_freefunction 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_connectionsfunction 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_connectionsfunction eventually calls into thesftp_disconnectfunction, which runs thessh_block_statemachfunction. 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.