From b42c53e442b211d0ded1d4c9abd18c74d29ed663 Mon Sep 17 00:00:00 2001 From: Nadeem Vawda Date: Sat, 23 Jul 2011 15:51:16 +0200 Subject: Issue #10883: Fix socket leaks in urllib.request. * ftpwrapper now uses reference counting to ensure that the underlying socket is closed when the ftpwrapper object is no longer in use * ftplib.FTP.ntransfercmd() now closes the socket if an error occurs Initial patch by Victor Stinner. --- Lib/ftplib.py | 56 +++++++++++++++++++++++++-------------------- Lib/test/test_urllib2.py | 1 + Lib/test/test_urllib2net.py | 1 + Lib/urllib.py | 27 ++++++++++++++++++---- Lib/urllib2.py | 9 +++++++- Misc/NEWS | 2 ++ 6 files changed, 65 insertions(+), 31 deletions(-) diff --git a/Lib/ftplib.py b/Lib/ftplib.py index 8713e47..c896433 100644 --- a/Lib/ftplib.py +++ b/Lib/ftplib.py @@ -325,33 +325,39 @@ class FTP: if self.passiveserver: host, port = self.makepasv() conn = socket.create_connection((host, port), self.timeout) - if rest is not None: - self.sendcmd("REST %s" % rest) - resp = self.sendcmd(cmd) - # Some servers apparently send a 200 reply to - # a LIST or STOR command, before the 150 reply - # (and way before the 226 reply). This seems to - # be in violation of the protocol (which only allows - # 1xx or error messages for LIST), so we just discard - # this response. - if resp[0] == '2': - resp = self.getresp() - if resp[0] != '1': - raise error_reply, resp + try: + if rest is not None: + self.sendcmd("REST %s" % rest) + resp = self.sendcmd(cmd) + # Some servers apparently send a 200 reply to + # a LIST or STOR command, before the 150 reply + # (and way before the 226 reply). This seems to + # be in violation of the protocol (which only allows + # 1xx or error messages for LIST), so we just discard + # this response. + if resp[0] == '2': + resp = self.getresp() + if resp[0] != '1': + raise error_reply, resp + except: + conn.close() + raise else: sock = self.makeport() - if rest is not None: - self.sendcmd("REST %s" % rest) - resp = self.sendcmd(cmd) - # See above. - if resp[0] == '2': - resp = self.getresp() - if resp[0] != '1': - raise error_reply, resp - conn, sockaddr = sock.accept() - if self.timeout is not _GLOBAL_DEFAULT_TIMEOUT: - conn.settimeout(self.timeout) - sock.close() + try: + if rest is not None: + self.sendcmd("REST %s" % rest) + resp = self.sendcmd(cmd) + # See above. + if resp[0] == '2': + resp = self.getresp() + if resp[0] != '1': + raise error_reply, resp + conn, sockaddr = sock.accept() + if self.timeout is not _GLOBAL_DEFAULT_TIMEOUT: + conn.settimeout(self.timeout) + finally: + sock.close() if resp[:3] == '150': # this is conditional in case we received a 125 size = parse150(resp) diff --git a/Lib/test/test_urllib2.py b/Lib/test/test_urllib2.py index e889bc3..e247315 100644 --- a/Lib/test/test_urllib2.py +++ b/Lib/test/test_urllib2.py @@ -611,6 +611,7 @@ class HandlerTests(unittest.TestCase): def retrfile(self, filename, filetype): self.filename, self.filetype = filename, filetype return StringIO.StringIO(self.data), len(self.data) + def close(self): pass class NullFTPHandler(urllib2.FTPHandler): def __init__(self, data): self.data = data diff --git a/Lib/test/test_urllib2net.py b/Lib/test/test_urllib2net.py index 867fcfa..5ffba87 100644 --- a/Lib/test/test_urllib2net.py +++ b/Lib/test/test_urllib2net.py @@ -231,6 +231,7 @@ class OtherNetworkTests(unittest.TestCase): handlers = [] cfh = urllib2.CacheFTPHandler() + self.addCleanup(cfh.clear_cache) cfh.setTimeout(1) handlers.append(cfh) diff --git a/Lib/urllib.py b/Lib/urllib.py index 62c08c9..ce90e91 100644 --- a/Lib/urllib.py +++ b/Lib/urllib.py @@ -850,13 +850,16 @@ class ftpwrapper: """Class used by open_ftp() for cache of open FTP connections.""" def __init__(self, user, passwd, host, port, dirs, - timeout=socket._GLOBAL_DEFAULT_TIMEOUT): + timeout=socket._GLOBAL_DEFAULT_TIMEOUT, + persistent=False): self.user = user self.passwd = passwd self.host = host self.port = port self.dirs = dirs self.timeout = timeout + self.refcount = 0 + self.keepalive = persistent self.init() def init(self): @@ -883,7 +886,7 @@ class ftpwrapper: # Try to retrieve as a file try: cmd = 'RETR ' + file - conn = self.ftp.ntransfercmd(cmd) + conn, retrlen = self.ftp.ntransfercmd(cmd) except ftplib.error_perm, reason: if str(reason)[:3] != '550': raise IOError, ('ftp error', reason), sys.exc_info()[2] @@ -903,11 +906,14 @@ class ftpwrapper: cmd = 'LIST ' + file else: cmd = 'LIST' - conn = self.ftp.ntransfercmd(cmd) + conn, retrlen = self.ftp.ntransfercmd(cmd) self.busy = 1 + ftpobj = addclosehook(conn.makefile('rb'), self.file_close) + self.refcount += 1 + conn.close() # Pass back both a suitably decorated object and a retrieval length - return (addclosehook(conn[0].makefile('rb'), - self.endtransfer), conn[1]) + return (ftpobj, retrlen) + def endtransfer(self): if not self.busy: return @@ -918,6 +924,17 @@ class ftpwrapper: pass def close(self): + self.keepalive = False + if self.refcount <= 0: + self.real_close() + + def file_close(self): + self.endtransfer() + self.refcount -= 1 + if self.refcount <= 0 and not self.keepalive: + self.real_close() + + def real_close(self): self.endtransfer() try: self.ftp.close() diff --git a/Lib/urllib2.py b/Lib/urllib2.py index 2641619..2bce745 100644 --- a/Lib/urllib2.py +++ b/Lib/urllib2.py @@ -1399,7 +1399,8 @@ class FTPHandler(BaseHandler): raise URLError, ('ftp error: %s' % msg), sys.exc_info()[2] def connect_ftp(self, user, passwd, host, port, dirs, timeout): - fw = ftpwrapper(user, passwd, host, port, dirs, timeout) + fw = ftpwrapper(user, passwd, host, port, dirs, timeout, + persistent=False) ## fw.ftp.set_debuglevel(1) return fw @@ -1448,3 +1449,9 @@ class CacheFTPHandler(FTPHandler): del self.timeout[k] break self.soonest = min(self.timeout.values()) + + def clear_cache(self): + for conn in self.cache.values(): + conn.close() + self.cache.clear() + self.timeout.clear() diff --git a/Misc/NEWS b/Misc/NEWS index 4bea4ea..7448838 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -37,6 +37,8 @@ Core and Builtins Library ------- +- Issue #10883: Fix socket leaks in urllib.request when using FTP. + - Issue #12592: Make Python build on OpenBSD 5 (and future major releases). - Issue #12372: POSIX semaphores are broken on AIX: don't use them. -- cgit v0.12