FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-140691: urllib.request: Close FTP control socket if data socket can't connect by encukou · Pull Request #140835 · python/cpython · GitHub

Repository navigation

Merged
3 changes: 3 additions & 0 deletions Lib/_py_warnings.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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):

Expand Down
59 changes: 49 additions & 10 deletions Lib/test/test_urllib2net.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import contextlib
import errno
import sysconfig
import unittest
from unittest import mock
Comment thread
cmaloney marked this conversation as resolved.
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
Expand Down Expand Up @@ -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):
Comment thread
cmaloney marked this conversation as resolved.
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')
Expand Down Expand Up @@ -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
Comment on lines -242 to -247

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(The new context (assertRaises) handles the expected error; printing information is nowadays covered by subTest)

else:
if f is not None:
try:
with time_out, \
socket_peer_reset, \
Expand Down
24 changes: 15 additions & 9 deletions Lib/urllib/request.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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,
Expand All @@ -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
Expand Down
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -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`.
Loading

Back | FazBrowse Home | New Git URL