Skip to content

Commit 0200016

Browse files
nvawdanvawda
committed
Merge: #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.
2 parents 7cd94a1 + 08f5f7a commit 0200016

4 files changed

Lines changed: 59 additions & 29 deletions

File tree

‎Lib/ftplib.py‎

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -343,33 +343,39 @@ def ntransfercmd(self, cmd, rest=None):
343343
host, port = self.makepasv()
344344
conn = socket.create_connection((host, port), self.timeout,
345345
source_address=self.source_address)
346-
if rest is not None:
347-
self.sendcmd("REST %s" % rest)
348-
resp = self.sendcmd(cmd)
349-
# Some servers apparently send a 200 reply to
350-
# a LIST or STOR command, before the 150 reply
351-
# (and way before the 226 reply). This seems to
352-
# be in violation of the protocol (which only allows
353-
# 1xx or error messages for LIST), so we just discard
354-
# this response.
355-
if resp[0] == '2':
356-
resp = self.getresp()
357-
if resp[0] != '1':
358-
raise error_reply(resp)
346+
try:
347+
if rest is not None:
348+
self.sendcmd("REST %s" % rest)
349+
resp = self.sendcmd(cmd)
350+
# Some servers apparently send a 200 reply to
351+
# a LIST or STOR command, before the 150 reply
352+
# (and way before the 226 reply). This seems to
353+
# be in violation of the protocol (which only allows
354+
# 1xx or error messages for LIST), so we just discard
355+
# this response.
356+
if resp[0] == '2':
357+
resp = self.getresp()
358+
if resp[0] != '1':
359+
raise error_reply(resp)
360+
except:
361+
conn.close()
362+
raise
359363
else:
360364
sock = self.makeport()
361-
if rest is not None:
362-
self.sendcmd("REST %s" % rest)
363-
resp = self.sendcmd(cmd)
364-
# See above.
365-
if resp[0] == '2':
366-
resp = self.getresp()
367-
if resp[0] != '1':
368-
raise error_reply(resp)
369-
conn, sockaddr = sock.accept()
370-
if self.timeout is not _GLOBAL_DEFAULT_TIMEOUT:
371-
conn.settimeout(self.timeout)
372-
sock.close()
365+
try:
366+
if rest is not None:
367+
self.sendcmd("REST %s" % rest)
368+
resp = self.sendcmd(cmd)
369+
# See above.
370+
if resp[0] == '2':
371+
resp = self.getresp()
372+
if resp[0] != '1':
373+
raise error_reply(resp)
374+
conn, sockaddr = sock.accept()
375+
if self.timeout is not _GLOBAL_DEFAULT_TIMEOUT:
376+
conn.settimeout(self.timeout)
377+
finally:
378+
sock.close()
373379
if resp[:3] == '150':
374380
# this is conditional in case we received a 125
375381
size = parse150(resp)

‎Lib/test/test_urllib2.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,6 +623,7 @@ def __init__(self, data): self.data = data
623623
def retrfile(self, filename, filetype):
624624
self.filename, self.filetype = filename, filetype
625625
return io.StringIO(self.data), len(self.data)
626+
def close(self): pass
626627

627628
class NullFTPHandler(urllib.request.FTPHandler):
628629
def __init__(self, data): self.data = data

‎Lib/test/test_urllib2net.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,7 @@ def _extra_handlers(self):
222222
handlers = []
223223

224224
cfh = urllib.request.CacheFTPHandler()
225+
self.addCleanup(cfh.clear_cache)
225226
cfh.setTimeout(1)
226227
handlers.append(cfh)
227228

‎Lib/urllib/request.py‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1371,8 +1371,8 @@ def ftp_open(self, req):
13711371
raise exc.with_traceback(sys.exc_info()[2])
13721372

13731373
def connect_ftp(self, user, passwd, host, port, dirs, timeout):
1374-
fw = ftpwrapper(user, passwd, host, port, dirs, timeout)
1375-
return fw
1374+
return ftpwrapper(user, passwd, host, port, dirs, timeout,
1375+
persistent=False)
13761376

13771377
class CacheFTPHandler(FTPHandler):
13781378
# XXX would be nice to have pluggable cache strategies
@@ -1421,6 +1421,13 @@ def check_cache(self):
14211421
break
14221422
self.soonest = min(list(self.timeout.values()))
14231423

1424+
def clear_cache(self):
1425+
for conn in self.cache.values():
1426+
conn.close()
1427+
self.cache.clear()
1428+
self.timeout.clear()
1429+
1430+
14241431
# Code move from the old urllib module
14251432

14261433
MAXFTPCACHE = 10 # Trim the ftp cache beyond this size
@@ -2144,13 +2151,16 @@ def noheaders():
21442151
class ftpwrapper:
21452152
"""Class used by open_ftp() for cache of open FTP connections."""
21462153

2147-
def __init__(self, user, passwd, host, port, dirs, timeout=None):
2154+
def __init__(self, user, passwd, host, port, dirs, timeout=None,
2155+
persistent=True):
21482156
self.user = user
21492157
self.passwd = passwd
21502158
self.host = host
21512159
self.port = port
21522160
self.dirs = dirs
21532161
self.timeout = timeout
2162+
self.refcount = 0
2163+
self.keepalive = persistent
21542164
self.init()
21552165

21562166
def init(self):
@@ -2201,7 +2211,8 @@ def retrfile(self, file, type):
22012211
conn, retrlen = self.ftp.ntransfercmd(cmd)
22022212
self.busy = 1
22032213

2204-
ftpobj = addclosehook(conn.makefile('rb'), self.endtransfer)
2214+
ftpobj = addclosehook(conn.makefile('rb'), self.file_close)
2215+
self.refcount += 1
22052216
conn.close()
22062217
# Pass back both a suitably decorated object and a retrieval length
22072218
return (ftpobj, retrlen)
@@ -2216,6 +2227,17 @@ def endtransfer(self):
22162227
pass
22172228

22182229
def close(self):
2230+
self.keepalive = False
2231+
if self.refcount <= 0:
2232+
self.real_close()
2233+
2234+
def file_close(self):
2235+
self.endtransfer()
2236+
self.refcount -= 1
2237+
if self.refcount <= 0 and not self.keepalive:
2238+
self.real_close()
2239+
2240+
def real_close(self):
22192241
self.endtransfer()
22202242
try:
22212243
self.ftp.close()

0 commit comments

Comments
 (0)