From 277cd165945cb6923e1946ee04944b497f6ec3e2 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 23 May 2018 10:45:15 -0400 Subject: [PATCH 01/11] Avoid race-prone allocate_tcp_port for some Tubs when possible create_tub on POSIX can pre-allocate a port safely instead. --- src/allmydata/test/test_introducer.py | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 7b7e29b96..f2497e934 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -314,18 +314,36 @@ class Server(unittest.TestCase): NICKNAME = u"n\u00EDickname-%s" # LATIN SMALL LETTER I WITH ACUTE +def foolscapEndpointForPortNumber(portnum): + if portnum is None: + from twisted.internet import reactor + from twisted.internet.interfaces import IReactorSocket + if IReactorSocket.providedBy(reactor): + from socket import socket, AF_INET + from twisted.internet.endpoints import AdoptedStreamServerEndpoint + s = socket() + s.setblocking(False) + s.bind(('', 0)) + portnum = s.getsockname()[1] + s.listen(3) + return (portnum, AdoptedStreamServerEndpoint(reactor, s.fileno(), AF_INET)) + else: + # Get a random port number and fall through. + portnum = iputil.allocate_tcp_port() + return (portnum, "tcp:%d" % (portnum,)) + + class SystemTestMixin(ServiceMixin, pollmixin.PollMixin): def create_tub(self, portnum=None): + portnum, endpoint = foolscapEndpointForPortNumber(portnum) tubfile = os.path.join(self.basedir, "tub.pem") self.central_tub = tub = Tub(certFile=tubfile) #tub.setOption("logLocalFailures", True) #tub.setOption("logRemoteFailures", True) tub.setOption("expose-remote-exception-types", False) tub.setServiceParent(self.parent) - if portnum is None: - portnum = iputil.allocate_tcp_port() - tub.listenOn("tcp:%d" % portnum) + tub.listenOn(endpoint) self.central_portnum = portnum tub.setLocation("localhost:%d" % self.central_portnum) From 711d63960d0fd39fb3d6070ff7b0b030e0222859 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 23 May 2018 10:47:30 -0400 Subject: [PATCH 02/11] Switch another test to the adoption method --- src/allmydata/test/test_introducer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index f2497e934..5c8185904 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -432,8 +432,8 @@ class SystemTest(SystemTestMixin, unittest.TestCase): #tub.setOption("logRemoteFailures", True) tub.setOption("expose-remote-exception-types", False) tub.setServiceParent(self.parent) - portnum = iputil.allocate_tcp_port() - tub.listenOn("tcp:%d" % portnum) + portnum, endpoint = foolscapEndpointForPortNumber(None) + tub.listenOn(endpoint) tub.setLocation("localhost:%d" % portnum) log.msg("creating client %d: %s" % (i, tub.getShortTubID())) From 62836b6858b82d4da1fb8c8f89911f92231e05fb Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 23 May 2018 10:47:41 -0400 Subject: [PATCH 03/11] Switch another test to the adoption method --- src/allmydata/test/test_introducer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 5c8185904..a3fb0a6fc 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -913,8 +913,8 @@ class NonV1Server(SystemTestMixin, unittest.TestCase): tub = Tub() tub.setOption("expose-remote-exception-types", False) tub.setServiceParent(self.parent) - portnum = iputil.allocate_tcp_port() - tub.listenOn("tcp:%d" % portnum) + portnum, endpoint = foolscapEndpointForPortNumber(None) + tub.listenOn(endpoint) tub.setLocation("localhost:%d" % portnum) c = IntroducerClient(tub, self.introducer_furl, From be6e45877098d24342d49b99b912097f7e476ce3 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 23 May 2018 10:50:54 -0400 Subject: [PATCH 04/11] Ensure the fd will be valid by the time we listen --- src/allmydata/test/test_introducer.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index a3fb0a6fc..88e146e6d 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -322,11 +322,17 @@ def foolscapEndpointForPortNumber(portnum): from socket import socket, AF_INET from twisted.internet.endpoints import AdoptedStreamServerEndpoint s = socket() - s.setblocking(False) - s.bind(('', 0)) - portnum = s.getsockname()[1] - s.listen(3) - return (portnum, AdoptedStreamServerEndpoint(reactor, s.fileno(), AF_INET)) + try: + s.setblocking(False) + s.bind(('', 0)) + portnum = s.getsockname()[1] + s.listen(3) + return ( + portnum, + AdoptedStreamServerEndpoint(reactor, os.dup(s.fileno()), AF_INET), + ) + finally: + s.close() else: # Get a random port number and fall through. portnum = iputil.allocate_tcp_port() From 8a5e2edb91d7d58095329e060ad3d874809b5474 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 23 May 2018 10:56:26 -0400 Subject: [PATCH 05/11] Also CLOEXEC the descriptor This avoids leaking it into any child processes that the tests might launch. --- src/allmydata/test/test_introducer.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 88e146e6d..1653b1d04 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -319,17 +319,21 @@ def foolscapEndpointForPortNumber(portnum): from twisted.internet import reactor from twisted.internet.interfaces import IReactorSocket if IReactorSocket.providedBy(reactor): + import fcntl from socket import socket, AF_INET from twisted.internet.endpoints import AdoptedStreamServerEndpoint s = socket() try: - s.setblocking(False) s.bind(('', 0)) portnum = s.getsockname()[1] - s.listen(3) + s.listen(1) + fd = os.dup(s.fileno()) + flags = fcntl.fcntl(fd, fcntl.F_GETFD) + flags = flags | os.O_NONBLOCK | fcntl.FD_CLOEXEC + fcntl.fcntl(fd, fcntl.F_SETFD, flags) return ( portnum, - AdoptedStreamServerEndpoint(reactor, os.dup(s.fileno()), AF_INET), + AdoptedStreamServerEndpoint(reactor, fd, AF_INET), ) finally: s.close() From 318eea05e32bcb7ae1331b4c7cd07836a7470369 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 23 May 2018 10:59:42 -0400 Subject: [PATCH 06/11] docs --- src/allmydata/test/test_introducer.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 1653b1d04..8b35aca03 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -315,10 +315,23 @@ class Server(unittest.TestCase): NICKNAME = u"n\u00EDickname-%s" # LATIN SMALL LETTER I WITH ACUTE def foolscapEndpointForPortNumber(portnum): + """ + Create an endpoint that can be passed to ``Tub.listen``. + + :param portnum: Either an integer port number indicating which TCP/IPv4 + port number the endpoint should bind or ``None`` to automatically + allocate such a port number. + + :return: A two-tuple of the integer port number allocated and a + Foolscap-compatible endpoint object. + """ if portnum is None: from twisted.internet import reactor from twisted.internet.interfaces import IReactorSocket if IReactorSocket.providedBy(reactor): + # On POSIX we can take this very safe approach of binding the + # actual socket to an address. Once the bind succeeds here, we're + # no longer subject to any future EADDRINUSE problems. import fcntl from socket import socket, AF_INET from twisted.internet.endpoints import AdoptedStreamServerEndpoint @@ -338,7 +351,10 @@ def foolscapEndpointForPortNumber(portnum): finally: s.close() else: - # Get a random port number and fall through. + # Get a random port number and fall through. This is necessary on + # Windows where Twisted doesn't offer IReactorSocket. This + # approach is error prone for the reasons described on + # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2787 portnum = iputil.allocate_tcp_port() return (portnum, "tcp:%d" % (portnum,)) From c491b1a7d48b0aea737ce5bcd0e7ae231a2393f9 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 8 Jun 2018 10:36:45 -0400 Subject: [PATCH 07/11] bring some imports up to the top --- src/allmydata/test/test_introducer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 8b35aca03..59bc54c1b 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -2,11 +2,13 @@ import os, re, itertools from base64 import b32decode import json +from socket import socket, AF_INET from twisted.trial import unittest from twisted.internet import defer, address from twisted.python import log from twisted.python.filepath import FilePath +from twisted.internet.endpoints import AdoptedStreamServerEndpoint from foolscap.api import Tub, Referenceable, fireEventually, flushEventualQueue from twisted.application import service @@ -333,8 +335,6 @@ def foolscapEndpointForPortNumber(portnum): # actual socket to an address. Once the bind succeeds here, we're # no longer subject to any future EADDRINUSE problems. import fcntl - from socket import socket, AF_INET - from twisted.internet.endpoints import AdoptedStreamServerEndpoint s = socket() try: s.bind(('', 0)) From d25693145c839750f5cd35b0fbba2c81d7da4199 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 8 Jun 2018 11:21:25 -0400 Subject: [PATCH 08/11] Factor a little more duplication out of the tests --- src/allmydata/test/test_introducer.py | 30 ++++++++++++++++----------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 59bc54c1b..88b4e71ef 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -358,20 +358,32 @@ def foolscapEndpointForPortNumber(portnum): portnum = iputil.allocate_tcp_port() return (portnum, "tcp:%d" % (portnum,)) +def listenOnUnused(tub, portnum=None): + """ + Start listening on an unused TCP port number with the given tub. + + :param portnum: Either an integer port number indicating which TCP/IPv4 + port number the endpoint should bind or ``None`` to automatically + allocate such a port number. + + :return: An integer indicating the TCP port number on which the tub is now + listening. + """ + portnum, endpoint = foolscapEndpointForPortNumber(portnum) + tub.listenOn(endpoint) + tub.setLocation("localhost:%d" % (portnum,)) + return portnum class SystemTestMixin(ServiceMixin, pollmixin.PollMixin): def create_tub(self, portnum=None): - portnum, endpoint = foolscapEndpointForPortNumber(portnum) tubfile = os.path.join(self.basedir, "tub.pem") self.central_tub = tub = Tub(certFile=tubfile) #tub.setOption("logLocalFailures", True) #tub.setOption("logRemoteFailures", True) tub.setOption("expose-remote-exception-types", False) tub.setServiceParent(self.parent) - tub.listenOn(endpoint) - self.central_portnum = portnum - tub.setLocation("localhost:%d" % self.central_portnum) + self.central_portnum = listenOnUnused(tub, portnum) class Queue(SystemTestMixin, unittest.TestCase): def test_queue_until_connected(self): @@ -458,10 +470,7 @@ class SystemTest(SystemTestMixin, unittest.TestCase): #tub.setOption("logRemoteFailures", True) tub.setOption("expose-remote-exception-types", False) tub.setServiceParent(self.parent) - portnum, endpoint = foolscapEndpointForPortNumber(None) - tub.listenOn(endpoint) - tub.setLocation("localhost:%d" % portnum) - + listenOnUnused(tub) log.msg("creating client %d: %s" % (i, tub.getShortTubID())) c = IntroducerClient(tub, self.introducer_furl, NICKNAME % str(i), @@ -939,10 +948,7 @@ class NonV1Server(SystemTestMixin, unittest.TestCase): tub = Tub() tub.setOption("expose-remote-exception-types", False) tub.setServiceParent(self.parent) - portnum, endpoint = foolscapEndpointForPortNumber(None) - tub.listenOn(endpoint) - tub.setLocation("localhost:%d" % portnum) - + listenOnUnused(tub) c = IntroducerClient(tub, self.introducer_furl, u"nickname-client", "version", "oldest", {}, fakeseq, FilePath(self.mktemp())) From 2c38b148bf6446a73042f27183dcb1511e0619c4 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 8 Jun 2018 13:12:46 -0400 Subject: [PATCH 09/11] Move the safe interface import to the top --- src/allmydata/test/test_introducer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 88b4e71ef..acc3c6f1e 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -9,6 +9,7 @@ from twisted.internet import defer, address from twisted.python import log from twisted.python.filepath import FilePath from twisted.internet.endpoints import AdoptedStreamServerEndpoint +from twisted.internet.interfaces import IReactorSocket from foolscap.api import Tub, Referenceable, fireEventually, flushEventualQueue from twisted.application import service @@ -329,7 +330,6 @@ def foolscapEndpointForPortNumber(portnum): """ if portnum is None: from twisted.internet import reactor - from twisted.internet.interfaces import IReactorSocket if IReactorSocket.providedBy(reactor): # On POSIX we can take this very safe approach of binding the # actual socket to an address. Once the bind succeeds here, we're From 97e0ad627b1c752d86856f1a68bcc63eb58e8b3e Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 8 Jun 2018 13:13:26 -0400 Subject: [PATCH 10/11] explain the buried reactor import, just in case --- src/allmydata/test/test_introducer.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index acc3c6f1e..bb8f02241 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -329,6 +329,8 @@ def foolscapEndpointForPortNumber(portnum): Foolscap-compatible endpoint object. """ if portnum is None: + # Bury this reactor import here to minimize the chances of it having + # the effect of installing the default reactor. from twisted.internet import reactor if IReactorSocket.providedBy(reactor): # On POSIX we can take this very safe approach of binding the From 1911b35499d5dc8ded25a9122f02454f0d6a19c0 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 8 Jun 2018 13:15:19 -0400 Subject: [PATCH 11/11] Get the fcntl import up to the top as well --- src/allmydata/test/test_introducer.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index bb8f02241..3d3d9e88f 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -8,6 +8,7 @@ from twisted.trial import unittest from twisted.internet import defer, address from twisted.python import log from twisted.python.filepath import FilePath +from twisted.python.reflect import requireModule from twisted.internet.endpoints import AdoptedStreamServerEndpoint from twisted.internet.interfaces import IReactorSocket @@ -26,6 +27,8 @@ from allmydata.client import create_client from allmydata.util import pollmixin, keyutil, idlib, fileutil, iputil, yamlutil import allmydata.test.common_util as testutil +fcntl = requireModule("fcntl") + class LoggingMultiService(service.MultiService): def log(self, msg, **kw): log.msg(msg, **kw) @@ -332,11 +335,10 @@ def foolscapEndpointForPortNumber(portnum): # Bury this reactor import here to minimize the chances of it having # the effect of installing the default reactor. from twisted.internet import reactor - if IReactorSocket.providedBy(reactor): + if fcntl is not None and IReactorSocket.providedBy(reactor): # On POSIX we can take this very safe approach of binding the # actual socket to an address. Once the bind succeeds here, we're # no longer subject to any future EADDRINUSE problems. - import fcntl s = socket() try: s.bind(('', 0))