gh-140691: urllib.request: Close FTP control socket if data socket can't connect (GH-140835)

Co-authored-by: codenamenam <[email protected]>

Author: encukou

Lib/_py_warnings.py

@@ -646,6 +646,9 @@ def __str__(self):
                     "line : %r}" % (self.message, self._category_name,
                                     self.filename, self.lineno, self.line))
 
+    def __repr__(self):
+        return f'<{type(self).__qualname__} {self}>'
+
 
 class catch_warnings(object):
 

Lib/test/test_urllib2net.py

@@ -1,9 +1,13 @@
+import contextlib
 import errno
+import sysconfig
 import unittest
+from unittest import mock
 from test import support
 from test.support import os_helper
 from test.support import socket_helper
 from test.support import ResourceDenied
+from test.support.warnings_helper import check_no_resource_warning
 
 import os
 import socket
@@ -143,6 +147,43 @@ def test_ftp(self):
             ]
         self._test_urls(urls, self._extra_handlers())
 
+    @support.requires_resource('walltime')
+    @unittest.skipIf(sysconfig.get_platform() == 'linux-ppc64le',
+                     'leaks on PPC64LE (gh-140691)')
+    def test_ftp_no_leak(self):
+        # gh-140691: When the data connection (but not control connection)
+        # cannot be made established, we shouldn't leave an open socket object.
+
+        class MockError(OSError):
+            pass
+
+        orig_create_connection = socket.create_connection
+        def patched_create_connection(address, *args, **kwargs):
+            """Simulate REJECTing connections to ports other than 21"""
+            host, port = address
+            if port != 21:
+                raise MockError()
+            return orig_create_connection(address, *args, **kwargs)
+
+        url = 'ftp://www.pythontest.net/README'
+        entry = url, None, urllib.error.URLError
+        no_cache_handlers = [urllib.request.FTPHandler()]
+        cache_handlers = self._extra_handlers()
+        with mock.patch('socket.create_connection', patched_create_connection):
+            with check_no_resource_warning(self):
+                # Try without CacheFTPHandler
+                self._test_urls([entry], handlers=no_cache_handlers,
+                                retry=False)
+            with check_no_resource_warning(self):
+                # Try with CacheFTPHandler (uncached)
+                self._test_urls([entry], cache_handlers, retry=False)
+            with check_no_resource_warning(self):
+                # Try with CacheFTPHandler (cached)
+                self._test_urls([entry], cache_handlers, retry=False)
+        # Try without the mock: the handler should not use a closed connection
+        with check_no_resource_warning(self):
+            self._test_urls([url], cache_handlers, retry=False)
+
     def test_file(self):
         TESTFN = os_helper.TESTFN
         f = open(TESTFN, 'w')
@@ -234,18 +275,16 @@ def _test_urls(self, urls, handlers, retry=True):
                 else:
                     req = expected_err = None
 
+                if expected_err:
+                    context = self.assertRaises(expected_err)
+                else:
+                    context = contextlib.nullcontext()
+
                 with socket_helper.transient_internet(url):
-                    try:
+                    f = None
+                    with context:
                         f = urlopen(url, req, support.INTERNET_TIMEOUT)
-                    # urllib.error.URLError is a subclass of OSError
-                    except OSError as err:
-                        if expected_err:
-                            msg = ("Didn't get expected error(s) %s for %s %s, got %s: %s" %
-                                   (expected_err, url, req, type(err), err))
-                            self.assertIsInstance(err, expected_err, msg)
-                        else:
-                            raise
-                    else:
+                    if f is not None:
                         try:
                             with time_out, \
                                  socket_peer_reset, \

Lib/urllib/request.py

@@ -1535,6 +1535,7 @@ def ftp_open(self, req):
         dirs, file = dirs[:-1], dirs[-1]
         if dirs and not dirs[0]:
             dirs = dirs[1:]
+        fw = None
         try:
             fw = self.connect_ftp(user, passwd, host, port, dirs, req.timeout)
             type = file and 'I' or 'D'
@@ -1552,8 +1553,12 @@ def ftp_open(self, req):
                 headers += "Content-length: %d\n" % retrlen
             headers = email.message_from_string(headers)
             return addinfourl(fp, headers, req.full_url)
-        except ftplib.all_errors as exp:
-            raise URLError(f"ftp error: {exp}") from exp
+        except Exception as exp:
+            if fw is not None and not fw.keepalive:
+                fw.close()
+            if isinstance(exp, ftplib.all_errors):
+                raise URLError(f"ftp error: {exp}") from exp
+            raise
 
     def connect_ftp(self, user, passwd, host, port, dirs, timeout):
         return ftpwrapper(user, passwd, host, port, dirs, timeout,
@@ -1577,14 +1582,15 @@ def setMaxConns(self, m):
 
     def connect_ftp(self, user, passwd, host, port, dirs, timeout):
         key = user, host, port, '/'.join(dirs), timeout
-        if key in self.cache:
-            self.timeout[key] = time.time() + self.delay
-        else:
-            self.cache[key] = ftpwrapper(user, passwd, host, port,
-                                         dirs, timeout)
-            self.timeout[key] = time.time() + self.delay
+        conn = self.cache.get(key)
+        if conn is None or not conn.keepalive:
+            if conn is not None:
+                conn.close()
+            conn = self.cache[key] = ftpwrapper(user, passwd, host, port,
+                                                dirs, timeout)
+        self.timeout[key] = time.time() + self.delay
         self.check_cache()
-        return self.cache[key]
+        return conn
 
     def check_cache(self):
         # first check for old ones

Misc/NEWS.d/next/Library/2025-10-31-15-06-26.gh-issue-140691.JzHGtg.rst

@@ -0,0 +1,3 @@
+In :mod:`urllib.request`, when opening a FTP URL fails because a data
+connection cannot be made, the control connection's socket is now closed to
+avoid a :exc:`ResourceWarning`.