gh-103443: Make the TLS shutdown of the FTP data connection best-effort

Doc/library/ftplib.rst

@@ -549,6 +549,22 @@ FTP_TLS objects
 
       Set up clear text data connection.
 
+   .. note::
+      The TLS shutdown of the data connection is best-effort.  Once a
+      transfer has finished exchanging its payload,
+      :meth:`~FTP.retrbinary`, :meth:`~FTP.retrlines`,
+      :meth:`~FTP.storbinary` and :meth:`~FTP.storlines` attempt to
+      complete a TLS shutdown handshake on the data socket, but limit it
+      to a few seconds and ignore the errors it may raise.  Servers are
+      not required to perform a TLS shutdown on a connection which is
+      created for a single transfer and closed immediately afterwards,
+      and some of them never answer it at all.
+
+   .. versionchanged:: 3.16
+      A failed or unresponsive TLS shutdown of the data connection no
+      longer raises an exception or blocks indefinitely once the transfer
+      itself has completed.
+
 
 Module variables
 ^^^^^^^^^^^^^^^^

Lib/ftplib.py

@@ -436,9 +436,8 @@ def retrbinary(self, cmd, callback, blocksize=8192, rest=None):
         with self.transfercmd(cmd, rest) as conn:
             while data := conn.recv(blocksize):
                 callback(data)
-            # shutdown ssl layer
             if _SSLSocket is not None and isinstance(conn, _SSLSocket):
-                conn.unwrap()
+                _shutdown_data_channel(conn)
         return self.voidresp()
 
     def retrlines(self, cmd, callback = None):
@@ -471,9 +470,8 @@ def retrlines(self, cmd, callback = None):
                 elif line[-1:] == '\n':
                     line = line[:-1]
                 callback(line)
-            # shutdown ssl layer
             if _SSLSocket is not None and isinstance(conn, _SSLSocket):
-                conn.unwrap()
+                _shutdown_data_channel(conn)
         return self.voidresp()
 
     def storbinary(self, cmd, fp, blocksize=8192, callback=None, rest=None):
@@ -497,9 +495,8 @@ def storbinary(self, cmd, fp, blocksize=8192, callback=None, rest=None):
                 conn.sendall(buf)
                 if callback:
                     callback(buf)
-            # shutdown ssl layer
             if _SSLSocket is not None and isinstance(conn, _SSLSocket):
-                conn.unwrap()
+                _shutdown_data_channel(conn)
         return self.voidresp()
 
     def storlines(self, cmd, fp, callback=None):
@@ -528,9 +525,8 @@ def storlines(self, cmd, fp, callback=None):
                 conn.sendall(buf)
                 if callback:
                     callback(buf)
-            # shutdown ssl layer
             if _SSLSocket is not None and isinstance(conn, _SSLSocket):
-                conn.unwrap()
+                _shutdown_data_channel(conn)
         return self.voidresp()
 
     def acct(self, password):
@@ -674,6 +670,33 @@ def close(self):
 else:
     _SSLSocket = ssl.SSLSocket
 
+    _DATA_SHUTDOWN_TIMEOUT = 5.0
+
+    def _shutdown_data_channel(conn, timeout=None):
+        """Perform a best-effort TLS shutdown of a data connection.
+
+        The data connection of an FTP transfer is ephemeral: it is created
+        by transfercmd() for one transfer and closed right afterwards.
+        Servers are not required to complete a TLS shutdown handshake on it
+        and many do not send close_notify at all (gh-77303, gh-78738,
+        gh-103443, gh-124850), which makes an unconditional unwrap() either
+        raise or block forever *after* the whole payload has already been
+        sent, turning a completed transfer into an apparent failure.  The
+        handshake is still attempted (well-behaved peers get a clean
+        close_notify), but it is time-limited and never propagates an
+        error.
+        """
+        if timeout is None:
+            timeout = _DATA_SHUTDOWN_TIMEOUT
+        try:
+            conn.settimeout(timeout)
+            conn.unwrap()
+        except (OSError, ValueError):
+            # ConnectionResetError, SSLEOFError, TimeoutError and
+            # ssl.SSLError are all OSError subclasses; ValueError covers
+            # "No SSL wrapper around ...".
+            pass
+
     class FTP_TLS(FTP):
         '''A FTP subclass which adds TLS support to FTP as described
         in RFC-4217.

Lib/test/test_ftplib.py

@@ -972,6 +972,107 @@ def test_data_connection(self):
                              LIST_DATA.encode(self.client.encoding))
         self.assertEqual(self.client.voidresp(), "226 transfer complete")
 
+    def _unreliable_data_connection(self, exc):
+        """Make unwrap() on the *client* data connection raise *exc*.
+
+        transfercmd() is wrapped so that unwrap() on the socket it returns
+        raises *exc* instead of performing the TLS shutdown.  Only the data
+        connection of the client is touched: the control connection and the
+        server side keep working normally, which is exactly the situation
+        reported in the bugs (the peer aborts or half-closes the ephemeral
+        data connection once the transfer is over).
+        """
+        original = self.client.transfercmd
+
+        def transfercmd(*args, **kwargs):
+            sock = original(*args, **kwargs)
+            real_unwrap = sock.unwrap
+
+            def unwrap():
+                # Shut the TLS layer down before reporting the error.  A
+                # peer that fails the shutdown has still received (or sent)
+                # the whole payload, but closing the data connection without
+                # the close_notify handshake lets the dummy server below
+                # mistake a partial TLS read for end-of-stream and drop the
+                # tail of the transfer.
+                try:
+                    real_unwrap()
+                except OSError:
+                    pass
+                raise exc
+
+            sock.unwrap = unwrap
+            return sock
+
+        return mock.patch.object(self.client, 'transfercmd', transfercmd)
+
+    def test_data_shutdown_error_is_ignored(self):
+        # gh-77303, gh-103443, gh-124850: the payload has been fully sent
+        # or received by the time the data connection is shut down, so a
+        # failure there must not turn a completed transfer into an error.
+        self.client.auth()
+        self.client.prot_p()
+        payload = RETR_DATA.encode(self.client.encoding)
+        errors = (
+            ConnectionResetError('connection reset by peer'),
+            ssl.SSLEOFError('EOF occurred in violation of protocol'),
+            ssl.SSLError('shutdown while in init'),
+            OSError(errno.ECONNRESET, 'Connection reset by peer'),
+            ValueError('No SSL wrapper around socket'),
+        )
+        for exc in errors:
+            with self.subTest(exc=exc):
+                with self._unreliable_data_connection(exc):
+                    received = []
+                    self.client.retrbinary('retr', received.append)
+                    self.assertEqual(b''.join(received), payload)
+
+                    self.client.storbinary('stor', io.BytesIO(payload))
+                    self.assertEqual(
+                        bytes(self.server.handler_instance.last_received_data),
+                        RETR_DATA.encode(self.server.encoding))
+
+    def test_data_shutdown_timeout_is_ignored(self):
+        # gh-78738: a peer which never answers close_notify makes an
+        # unconditional unwrap() block forever on a socket whose whole
+        # purpose is over.  The shutdown has to be time-limited, and the
+        # resulting TimeoutError has to be swallowed.
+        self.client.auth()
+        self.client.prot_p()
+        payload = RETR_DATA.encode(self.client.encoding)
+
+        original = self.client.transfercmd
+
+        def transfercmd(*args, **kwargs):
+            sock = original(*args, **kwargs)
+            original_timeout = sock.gettimeout()
+
+            def unwrap():
+                shutdown_timeout = sock.gettimeout()
+                if shutdown_timeout == original_timeout:
+                    raise AssertionError(
+                        'data connection shutdown is unbounded: unwrap() is '
+                        'called without lowering the socket timeout first')
+                # Emulate what _ssl does when the peer stays silent.
+                time.sleep(shutdown_timeout)
+                raise TimeoutError('the handshake operation timed out')
+
+            sock.unwrap = unwrap
+            return sock
+
+        with mock.patch.object(ftplib, '_DATA_SHUTDOWN_TIMEOUT', 0.1,
+                               create=True):
+            with mock.patch.object(self.client, 'transfercmd', transfercmd):
+                start = time.monotonic()
+                received = []
+                self.client.retrbinary('retr', received.append)
+                self.assertEqual(b''.join(received), payload)
+
+                self.client.storbinary('stor', io.BytesIO(payload))
+                self.assertEqual(
+                    bytes(self.server.handler_instance.last_received_data),
+                    RETR_DATA.encode(self.server.encoding))
+                self.assertLess(time.monotonic() - start, TIMEOUT)
     def test_login(self):
         # login() is supposed to implicitly secure the control connection
         self.assertNotIsInstance(self.client.sock, ssl.SSLSocket)

Misc/NEWS.d/next/Library/2026-10-07-11-33-20.gh-issue-103443.hU.rst

@@ -0,0 +1,8 @@
+:func:`ftplib.FTP.retrbinary`, :func:`ftplib.FTP.retrlines`,
+:func:`ftplib.FTP.storbinary` and :func:`ftplib.FTP.storlines` no longer let
+the TLS shutdown of the data connection turn an already completed transfer
+into an exception or into an indefinite hang.  The shutdown handshake is
+still attempted, but it now runs with a short timeout and the errors a peer
+can cause there -- such as :exc:`ConnectionResetError` or
+:class:`ssl.SSLEOFError` when it does not complete the handshake on a data
+connection that only exists for a single transfer -- are ignored.