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

gh-108342: Make ssl TestPreHandshakeClose more reliable (#108370) · python/cpython@592bacb · GitHub

  1. python
  2. cpython

Repository navigation

Commit 592bacb

Browse files
vstinner
authored
gh-108342: Make ssl TestPreHandshakeClose more reliable (#108370)
* In preauth tests of test_ssl, explicitly break reference cycles invoving SingleConnectionTestServerThread to make sure that the thread is deleted. Otherwise, the test marks the environment as altered because the threading module sees a "dangling thread" (SingleConnectionTestServerThread). This test leak was introduced by the test added for the fix of issue gh-108310. * Use support.SHORT_TIMEOUT instead of hardcoded 1.0 or 2.0 seconds timeout. * SingleConnectionTestServerThread.run() catchs TimeoutError * Fix a race condition (missing synchronization) in test_preauth_data_to_tls_client(): the server now waits until the client connect() completed in call_after_accept(). * test_https_client_non_tls_response_ignored() calls server.join() explicitly. * Replace "localhost" with server.listener.getsockname()[0].
1 parent ec3527d commit 592bacb

1 file changed

Lines changed: 71 additions & 31 deletions

File tree

‎Lib/test/test_ssl.py‎

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -4672,12 +4672,16 @@ class TestPreHandshakeClose(unittest.TestCase):
46724672

46734673
class SingleConnectionTestServerThread(threading.Thread):
46744674

4675-
def __init__(self, *, name, call_after_accept):
4675+
def __init__(self, *, name, call_after_accept, timeout=None):
46764676
self.call_after_accept = call_after_accept
46774677
self.received_data = b'' # set by .run()
46784678
self.wrap_error = None # set by .run()
46794679
self.listener = None # set by .start()
46804680
self.port = None # set by .start()
4681+
if timeout is None:
4682+
self.timeout = support.SHORT_TIMEOUT
4683+
else:
4684+
self.timeout = timeout
46814685
super().__init__(name=name)
46824686

46834687
def __enter__(self):
@@ -4700,13 +4704,19 @@ def start(self):
47004704
self.ssl_ctx.load_cert_chain(certfile=ONLYCERT, keyfile=ONLYKEY)
47014705
self.listener = socket.socket()
47024706
self.port = socket_helper.bind_port(self.listener)
4703-
self.listener.settimeout(2.0)
4707+
self.listener.settimeout(self.timeout)
47044708
self.listener.listen(1)
47054709
super().start()
47064710

47074711
def run(self):
4708-
conn, address = self.listener.accept()
4709-
self.listener.close()
4712+
try:
4713+
conn, address = self.listener.accept()
4714+
except TimeoutError:
4715+
# on timeout, just close the listener
4716+
return
4717+
finally:
4718+
self.listener.close()
4719+
47104720
with conn:
47114721
if self.call_after_accept(conn):
47124722
return
@@ -4734,8 +4744,13 @@ def non_linux_skip_if_other_okay_error(self, err):
47344744
# we're specifically trying to test. The way this test is written
47354745
# is known to work on Linux. We'll skip it anywhere else that it
47364746
# does not present as doing so.
4737-
self.skipTest(f"Could not recreate conditions on {sys.platform}:"
4738-
f" {err=}")
4747+
try:
4748+
self.skipTest(f"Could not recreate conditions on {sys.platform}:"
4749+
f" {err=}")
4750+
finally:
4751+
# gh-108342: Explicitly break the reference cycle
4752+
err = None
4753+
47394754
# If maintaining this conditional winds up being a problem.
47404755
# just turn this into an unconditional skip anything but Linux.
47414756
# The important thing is that our CI has the logic covered.
@@ -4746,7 +4761,7 @@ def test_preauth_data_to_tls_server(self):
47464761

47474762
def call_after_accept(unused):
47484763
server_accept_called.set()
4749-
if not ready_for_server_wrap_socket.wait(2.0):
4764+
if not ready_for_server_wrap_socket.wait(support.SHORT_TIMEOUT):
47504765
raise RuntimeError("wrap_socket event never set, test may fail.")
47514766
return False # Tell the server thread to continue.
47524767

@@ -4767,20 +4782,31 @@ def call_after_accept(unused):
47674782

47684783
ready_for_server_wrap_socket.set()
47694784
server.join()
4785+
47704786
wrap_error = server.wrap_error
4771-
self.assertEqual(b"", server.received_data)
4772-
self.assertIsInstance(wrap_error, OSError) # All platforms.
4773-
self.non_linux_skip_if_other_okay_error(wrap_error)
4774-
self.assertIsInstance(wrap_error, ssl.SSLError)
4775-
self.assertIn("before TLS handshake with data", wrap_error.args[1])
4776-
self.assertIn("before TLS handshake with data", wrap_error.reason)
4777-
self.assertNotEqual(0, wrap_error.args[0])
4778-
self.assertIsNone(wrap_error.library, msg="attr must exist")
4787+
server.wrap_error = None
4788+
try:
4789+
self.assertEqual(b"", server.received_data)
4790+
self.assertIsInstance(wrap_error, OSError) # All platforms.
4791+
self.non_linux_skip_if_other_okay_error(wrap_error)
4792+
self.assertIsInstance(wrap_error, ssl.SSLError)
4793+
self.assertIn("before TLS handshake with data", wrap_error.args[1])
4794+
self.assertIn("before TLS handshake with data", wrap_error.reason)
4795+
self.assertNotEqual(0, wrap_error.args[0])
4796+
self.assertIsNone(wrap_error.library, msg="attr must exist")
4797+
finally:
4798+
# gh-108342: Explicitly break the reference cycle
4799+
wrap_error = None
4800+
server = None
47794801

47804802
def test_preauth_data_to_tls_client(self):
4803+
server_can_continue_with_wrap_socket = threading.Event()
47814804
client_can_continue_with_wrap_socket = threading.Event()
47824805

47834806
def call_after_accept(conn_to_client):
4807+
if not server_can_continue_with_wrap_socket.wait(support.SHORT_TIMEOUT):
4808+
print("ERROR: test client took too long")
4809+
47844810
# This forces an immediate connection close via RST on .close().
47854811
set_socket_so_linger_on_with_zero_timeout(conn_to_client)
47864812
conn_to_client.send(
@@ -4800,8 +4826,10 @@ def call_after_accept(conn_to_client):
48004826

48014827
with socket.socket() as client:
48024828
client.connect(server.listener.getsockname())
4803-
if not client_can_continue_with_wrap_socket.wait(2.0):
4804-
self.fail("test server took too long.")
4829+
server_can_continue_with_wrap_socket.set()
4830+
4831+
if not client_can_continue_with_wrap_socket.wait(support.SHORT_TIMEOUT):
4832+
self.fail("test server took too long")
48054833
ssl_ctx = ssl.create_default_context()
48064834
try:
48074835
tls_client = ssl_ctx.wrap_socket(
@@ -4815,24 +4843,31 @@ def call_after_accept(conn_to_client):
48154843
tls_client.close()
48164844

48174845
server.join()
4818-
self.assertEqual(b"", received_data)
4819-
self.assertIsInstance(wrap_error, OSError) # All platforms.
4820-
self.non_linux_skip_if_other_okay_error(wrap_error)
4821-
self.assertIsInstance(wrap_error, ssl.SSLError)
4822-
self.assertIn("before TLS handshake with data", wrap_error.args[1])
4823-
self.assertIn("before TLS handshake with data", wrap_error.reason)
4824-
self.assertNotEqual(0, wrap_error.args[0])
4825-
self.assertIsNone(wrap_error.library, msg="attr must exist")
4846+
try:
4847+
self.assertEqual(b"", received_data)
4848+
self.assertIsInstance(wrap_error, OSError) # All platforms.
4849+
self.non_linux_skip_if_other_okay_error(wrap_error)
4850+
self.assertIsInstance(wrap_error, ssl.SSLError)
4851+
self.assertIn("before TLS handshake with data", wrap_error.args[1])
4852+
self.assertIn("before TLS handshake with data", wrap_error.reason)
4853+
self.assertNotEqual(0, wrap_error.args[0])
4854+
self.assertIsNone(wrap_error.library, msg="attr must exist")
4855+
finally:
4856+
# gh-108342: Explicitly break the reference cycle
4857+
wrap_error = None
4858+
server = None
48264859

48274860
def test_https_client_non_tls_response_ignored(self):
4828-
48294861
server_responding = threading.Event()
48304862

48314863
class SynchronizedHTTPSConnection(http.client.HTTPSConnection):
48324864
def connect(self):
4865+
# Call clear text HTTP connect(), not the encrypted HTTPS (TLS)
4866+
# connect(): wrap_socket() is called manually below.
48334867
http.client.HTTPConnection.connect(self)
4868+
48344869
# Wait for our fault injection server to have done its thing.
4835-
if not server_responding.wait(1.0) and support.verbose:
4870+
if not server_responding.wait(support.SHORT_TIMEOUT) and support.verbose:
48364871
sys.stdout.write("server_responding event never set.")
48374872
self.sock = self._context.wrap_socket(
48384873
self.sock, server_hostname=self.host)
@@ -4847,28 +4882,33 @@ def call_after_accept(conn_to_client):
48474882
server_responding.set()
48484883
return True # Tell the server to stop.
48494884

4885+
timeout = 2.0
48504886
server = self.SingleConnectionTestServerThread(
48514887
call_after_accept=call_after_accept,
4852-
name="non_tls_http_RST_responder")
4888+
name="non_tls_http_RST_responder",
4889+
timeout=timeout)
48534890
self.enterContext(server) # starts it & unittest.TestCase stops it.
48544891
# Redundant; call_after_accept sets SO_LINGER on the accepted conn.
48554892
set_socket_so_linger_on_with_zero_timeout(server.listener)
48564893

48574894
connection = SynchronizedHTTPSConnection(
4858-
f"localhost",
4895+
server.listener.getsockname()[0],
48594896
port=server.port,
48604897
context=ssl.create_default_context(),
4861-
timeout=2.0,
4898+
timeout=timeout,
48624899
)
4900+
48634901
# There are lots of reasons this raises as desired, long before this
48644902
# test was added. Sending the request requires a successful TLS wrapped
48654903
# socket; that fails if the connection is broken. It may seem pointless
48664904
# to test this. It serves as an illustration of something that we never
48674905
# want to happen... properly not happening.
4868-
with self.assertRaises(OSError) as err_ctx:
4906+
with self.assertRaises(OSError):
48694907
connection.request("HEAD", "/test", headers={"Host": "localhost"})
48704908
response = connection.getresponse()
48714909

4910+
server.join()
4911+
48724912

48734913
class TestEnumerations(unittest.TestCase):
48744914

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL