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.
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 @@
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 @@
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 @@
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 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 @@
# 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 @@
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 @@
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 @@
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 @@
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 @@
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.