From 7b8bb927e75c07caad6a00473ad25521486ed4d3 Mon Sep 17 00:00:00 2001 From: yoav Date: Thu, 4 Mar 2021 10:44:17 +0200 Subject: Close socket on failure As mentioned in socket docs: "Sockets are automatically closed when they are garbage-collected, but it is recommended to close() them explicitly, or to use a with statement around them." Resolve #1126 --- paramiko/client.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/paramiko/client.py b/paramiko/client.py index 80c956cd..ae70ac0b 100644 --- a/paramiko/client.py +++ b/paramiko/client.py @@ -350,6 +350,9 @@ class SSHClient(ClosingContextManager): # Break out of the loop on success break except socket.error as e: + # As mentioned in socket docs - it is better to close sockets explicitly + if sock: + sock.close() # Raise anything that isn't a straight up connection error # (such as a resolution error) if e.errno not in (ECONNREFUSED, EHOSTUNREACH): -- cgit v1.2.1 From f6a64459a6f010f915ff0ec17cfb42dbc39e996d Mon Sep 17 00:00:00 2001 From: yoav Date: Thu, 4 Mar 2021 10:54:45 +0200 Subject: split comment to lines --- paramiko/client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/paramiko/client.py b/paramiko/client.py index ae70ac0b..c3cbcb9d 100644 --- a/paramiko/client.py +++ b/paramiko/client.py @@ -350,7 +350,8 @@ class SSHClient(ClosingContextManager): # Break out of the loop on success break except socket.error as e: - # As mentioned in socket docs - it is better to close sockets explicitly + # As mentioned in socket docs it is better + # to close sockets explicitly if sock: sock.close() # Raise anything that isn't a straight up connection error -- cgit v1.2.1 From 5a2dc1c20dc8ab89993d2db3141c9f9998b3b4e4 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Fri, 3 Jun 2022 19:33:23 -0400 Subject: Changelog re #1822, closes #1822 --- sites/www/changelog.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index af1cef05..e82a65b1 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,13 @@ Changelog ========= +- :bug:`1822` (via, and relating to, far too many other issues to mention here) + Update `~paramiko.client.SSHClient` so it explicitly closes its wrapped + socket object upon encountering socket errors at connection time. This should + help somewhat with certain classes of memory leaks, resource warnings, and/or + errors (though we hasten to remind everyone that Client and Transport have + their own ``.close()`` methods for use in non-error situations!). Patch + courtesy of ``@YoavCohen``. - :support:`1985` Add ``six`` explicitly to install-requires; it snuck into active use at some point but has only been indicated by transitive dependency on ``bcrypt`` until they somewhat-recently dropped it. This will be -- cgit v1.2.1 From bc4f2703d35fc639b7dff9ebf66902261f5087de Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Fri, 3 Jun 2022 19:52:40 -0400 Subject: Add test proving basic behavior of #1822 --- tests/test_client.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_client.py b/tests/test_client.py index f14aac23..a2e23b39 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -33,6 +33,7 @@ import warnings import weakref from tempfile import mkstemp +import pytest from pytest_relaxed import raises from mock import patch, Mock @@ -430,6 +431,23 @@ class SSHClientTest(ClientTest): assert p() is None + @patch("paramiko.client.socket.socket") + @patch("paramiko.client.socket.getaddrinfo") + def test_closes_socket_on_socket_errors(self, getaddrinfo, mocket): + getaddrinfo.return_value = ( + ("irrelevant", None, None, None, "whatever"), + ) + + class SocksToBeYou(socket.error): + pass + + my_socket = mocket.return_value + my_socket.connect.side_effect = SocksToBeYou + client = SSHClient() + with pytest.raises(SocksToBeYou): + client.connect(hostname="nope") + my_socket.close.assert_called_once_with() + def test_client_can_be_used_as_context_manager(self): """ verify that an SSHClient can be used a context manager -- cgit v1.2.1