From b8ab94b976383435dc6a8ad4c0f7df3377641fa1 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Tue, 28 Apr 2026 19:07:49 -0700 Subject: [PATCH] test(self-talk): Replace superstitious and timing-racy sleep() with proper primitives Eight fork-based self-talk / IO tests used hardcoded sleep() calls as ad-hoc synchronization between the parent (TLS server) and child (TLS client) processes. Three categories of sleeps were present: 1. Superstitious pre-negotiate sleeps: "give the server a chance to listen". Unnecessary because the socketpair underlying the io_pair is valid from the moment of fork(); the kernel buffers writes until the parent starts reading. 2. Superstitious pre-cleanup sleeps: "give the server a chance to avoid a sigpipe". Unnecessary because SIGPIPE is ignored process-wide via signal(SIGPIPE, SIG_IGN) in s2n_io_pair_init, and the disposition is inherited across fork(). 3. Load-bearing sleeps implementing ad-hoc synchronization. These were either a race (parent's pre-SIGCONT sleep in nonblocking_test, which could fire before the child had called raise(SIGSTOP)) or a guess at how long the other process would take to reach a given state (broken_pipe_test's sleeps around the half-closed-pipe assertions). Both flavors are replaced with proper primitives. Changes per file: - s2n_self_talk_client_hello_cb_test.c: removed 2 superstitious sleeps (pre-negotiate and pre-cleanup). - s2n_self_talk_nonblocking_test.c: removed 2 superstitious pre-negotiate sleeps; replaced the parent's pre-SIGCONT sleep with waitpid(pid, &status, WUNTRACED). WUNTRACED causes waitpid to return when the child transitions to the stopped state, eliminating a real race where SIGCONT could arrive before SIGSTOP. - s2n_self_talk_broken_pipe_test.c: removed 2 superstitious sleeps; replaced the two load-bearing sleeps (child's sleep(2) for "let parent observe EPIPE" and parent's sleep(1) for "let child shut down read side") with a pair of control pipes. The child writes to ready_pipe after shutting down its read side; parent blocks on read(ready_pipe) before its s2n_send. Parent writes to done_pipe after completing its assertions; child blocks on read(done_pipe) before teardown. Fully deterministic handoff. - s2n_self_talk_session_id_test.c: removed 4 superstitious sleeps (1 pre-negotiate + 3 pre-cleanup). - s2n_self_talk_tls13_test.c: removed 2 superstitious sleeps. - s2n_self_talk_tls12_test.c: removed 2 superstitious sleeps. - s2n_self_talk_alerts_test.c: removed 1 superstitious pre-negotiate sleep (called 3 times via the outer loop). - s2n_mem_allocator_test.c: removed 2 superstitious sleeps (called twice via the outer loop). All eight tests were run 100x in a flakiness loop after changes with no failures (800 total runs). Runtime on an M4 MacBook Pro: - s2n_self_talk_client_hello_cb_test: 19.54s -> 2.81s (~7x faster) - s2n_self_talk_nonblocking_test: 19.25s -> 4.75s (~4x faster) - s2n_self_talk_broken_pipe_test: 11.36s -> 2.88s (~4x faster) - s2n_self_talk_tls12_test: 8.54s -> 5.41s (~1.6x faster) - s2n_self_talk_session_id_test: 6.12s -> 2.69s (~2.3x faster) - s2n_self_talk_tls13_test: 5.56s -> 4.08s (~1.4x faster) - s2n_self_talk_alerts_test: 5.76s -> 2.86s (~2.0x faster) - s2n_mem_allocator_test: 9.14s -> 5.45s (~1.7x faster) Refs: https://github.com/aws/s2n-tls/issues/5704 --- tests/unit/s2n_mem_allocator_test.c | 8 +-- tests/unit/s2n_self_talk_alerts_test.c | 4 +- tests/unit/s2n_self_talk_broken_pipe_test.c | 66 ++++++++++++++++--- .../unit/s2n_self_talk_client_hello_cb_test.c | 10 +-- tests/unit/s2n_self_talk_nonblocking_test.c | 20 ++++-- tests/unit/s2n_self_talk_session_id_test.c | 16 ++--- tests/unit/s2n_self_talk_tls12_test.c | 8 +-- tests/unit/s2n_self_talk_tls13_test.c | 8 +-- 8 files changed, 98 insertions(+), 42 deletions(-) diff --git a/tests/unit/s2n_mem_allocator_test.c b/tests/unit/s2n_mem_allocator_test.c index bd4bdf0f294..eea84b58809 100644 --- a/tests/unit/s2n_mem_allocator_test.c +++ b/tests/unit/s2n_mem_allocator_test.c @@ -82,8 +82,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) struct s2n_config *config = NULL; s2n_blocked_status blocked; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair is valid from fork; the kernel buffers writes until the + * parent reads, so no pre-negotiate sleep is needed. */ conn = s2n_connection_new(S2N_CLIENT); config = s2n_config_new(); @@ -138,8 +138,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) s2n_connection_free(conn); s2n_config_free(config); - /* Give the server a chance to a void a sigpipe */ - sleep(1); + /* SIGPIPE is inherited as SIG_IGN across fork via s2n_io_pair_init, so + * no pre-cleanup sleep is needed. */ s2n_io_pair_close_one_end(io_pair, S2N_CLIENT); diff --git a/tests/unit/s2n_self_talk_alerts_test.c b/tests/unit/s2n_self_talk_alerts_test.c index cf596ba5d5c..c6edfda4501 100644 --- a/tests/unit/s2n_self_talk_alerts_test.c +++ b/tests/unit/s2n_self_talk_alerts_test.c @@ -50,8 +50,8 @@ int mock_client(struct s2n_test_io_pair *io_pair, s2n_alert_behavior alert_behav int result = 0; int rc = 0; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair is valid from fork; the kernel buffers writes until the + * parent reads, so no pre-negotiate sleep is needed. */ conn = s2n_connection_new(S2N_CLIENT); config = s2n_config_new(); diff --git a/tests/unit/s2n_self_talk_broken_pipe_test.c b/tests/unit/s2n_self_talk_broken_pipe_test.c index 87b237d11d2..cdb942089c5 100644 --- a/tests/unit/s2n_self_talk_broken_pipe_test.c +++ b/tests/unit/s2n_self_talk_broken_pipe_test.c @@ -30,14 +30,15 @@ static const char *certificate_paths[SUPPORTED_CERTIFICATE_FORMATS] = { S2N_RSA_2048_PKCS1_CERT_CHAIN, S2N_RSA_2048_PKCS8_CERT_CHAIN }; static const char *private_key_paths[SUPPORTED_CERTIFICATE_FORMATS] = { S2N_RSA_2048_PKCS1_KEY, S2N_RSA_2048_PKCS8_KEY }; -void mock_client(struct s2n_test_io_pair *io_pair) +void mock_client(struct s2n_test_io_pair *io_pair, int ready_pipe_w, int done_pipe_r) { struct s2n_connection *conn = NULL; struct s2n_config *config = NULL; s2n_blocked_status blocked; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair underlying io_pair is valid from the moment of fork, + * so the child can start the handshake immediately. The kernel buffers + * writes until the parent is ready to read. */ conn = s2n_connection_new(S2N_CLIENT); config = s2n_config_new(); @@ -64,8 +65,23 @@ void mock_client(struct s2n_test_io_pair *io_pair) /* Close client read fd to mock half closed pipe at server side */ s2n_io_pair_shutdown_one_end(io_pair, S2N_CLIENT, SHUT_RD); #endif - /* Give server a chance to send data on broken pipe */ - sleep(2); + + /* Signal the parent: the read end of the socketpair is now shut down, + * so the parent's next s2n_send is guaranteed to observe EPIPE. */ + uint8_t ready = 1; + if (write(ready_pipe_w, &ready, 1) != 1) { + exit(1); + } + close(ready_pipe_w); + + /* Wait for the parent to finish its broken-pipe assertions before + * continuing teardown. Without this, the child could race ahead and + * close the socket entirely while the parent is mid-assertion. */ + uint8_t done = 0; + if (read(done_pipe_r, &done, 1) != 1) { + exit(1); + } + close(done_pipe_r); s2n_shutdown(conn, &blocked); @@ -79,8 +95,9 @@ void mock_client(struct s2n_test_io_pair *io_pair) exit(1); } - /* Give the server a chance to avoid a sigpipe */ - sleep(1); + /* The parent has SIGPIPE ignored (via s2n_io_pair_init), and the child + * inherits that signal disposition through fork(), so there's no need to + * sleep before tearing down the other end of the socketpair. */ s2n_io_pair_shutdown_one_end(io_pair, S2N_CLIENT, SHUT_WR); s2n_io_pair_close_one_end(io_pair, S2N_CLIENT); @@ -107,16 +124,35 @@ int main(int argc, char **argv) DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close); EXPECT_SUCCESS(s2n_io_pair_init(&io_pair)); + /* Create control pipes for child<->parent synchronization. The child + * writes to ready_pipe after shutting down its read side, so the + * parent can deterministically observe EPIPE on its next s2n_send. + * The parent writes to done_pipe after completing its assertions, so + * the child knows when it's safe to tear down the socket. This + * replaces two timing-dependent sleep() calls. */ + int ready_pipe[2] = { -1, -1 }; + int done_pipe[2] = { -1, -1 }; + EXPECT_EQUAL(pipe(ready_pipe), 0); + EXPECT_EQUAL(pipe(done_pipe), 0); + /* Create a child process */ pid_t pid = fork(); if (pid == 0) { /* This is the client process, close the server end of the pipe */ EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_SERVER)); + /* Close the pipe ends the child doesn't use */ + close(ready_pipe[0]); + close(done_pipe[1]); + /* Write the fragmented hello message */ - mock_client(&io_pair); + mock_client(&io_pair, ready_pipe[1], done_pipe[0]); } + /* Close the pipe ends the parent doesn't use */ + close(ready_pipe[1]); + close(done_pipe[0]); + /* This is the server process, close the client end of the pipe */ EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_CLIENT)); @@ -146,8 +182,12 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_negotiate(conn, &blocked)); EXPECT_EQUAL(conn->actual_protocol_version, S2N_TLS12); - /* Give client a chance to close pipe at the receiving end */ - sleep(1); + /* Wait for the child to shut down its read side of the socketpair. + * Once this read returns, the next s2n_send on the server conn is + * guaranteed to observe EPIPE. */ + uint8_t ready = 0; + EXPECT_EQUAL(read(ready_pipe[0], &ready, 1), 1); + EXPECT_SUCCESS(close(ready_pipe[0])); char buffer[1]; /* Fist flush on half closed pipe should get EPIPE */ ssize_t w = s2n_send(conn, buffer, 1, &blocked); @@ -161,6 +201,12 @@ int main(int argc, char **argv) EXPECT_EQUAL(s2n_errno, S2N_ERR_IO); EXPECT_EQUAL(errno, 0); + /* Signal the child that the assertions are complete so it can tear + * down its socket and exit. */ + uint8_t done = 1; + EXPECT_EQUAL(write(done_pipe[1], &done, 1), 1); + EXPECT_SUCCESS(close(done_pipe[1])); + EXPECT_SUCCESS(s2n_connection_free(conn)); for (int cert = 0; cert < SUPPORTED_CERTIFICATE_FORMATS; cert++) { EXPECT_SUCCESS(s2n_cert_chain_and_key_free(chain_and_keys[cert])); diff --git a/tests/unit/s2n_self_talk_client_hello_cb_test.c b/tests/unit/s2n_self_talk_client_hello_cb_test.c index 84ef83f33a5..21c5f2b1096 100644 --- a/tests/unit/s2n_self_talk_client_hello_cb_test.c +++ b/tests/unit/s2n_self_talk_client_hello_cb_test.c @@ -48,8 +48,9 @@ int mock_client(struct s2n_test_io_pair *io_pair, int expect_failure, int expect int rc = 0; const char *protocols[] = { "h2", "http/1.1" }; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair underlying io_pair is valid from the moment of fork, so + * the child can start the handshake immediately. The kernel will buffer + * writes until the parent starts reading. No pre-negotiate sleep needed. */ conn = s2n_connection_new(S2N_CLIENT); config = s2n_config_new(); @@ -96,8 +97,9 @@ int mock_client(struct s2n_test_io_pair *io_pair, int expect_failure, int expect s2n_connection_free(conn); s2n_config_free(config); - /* Give the server a chance to a void a sigpipe */ - sleep(1); + /* SIGPIPE is handled at the test's signal level (or ignored via + * SIGPIPE policy), so no post-shutdown sleep is needed to avoid a race + * with the server trying to send while we tear down. */ s2n_cleanup(); s2n_io_pair_close_one_end(io_pair, S2N_CLIENT); diff --git a/tests/unit/s2n_self_talk_nonblocking_test.c b/tests/unit/s2n_self_talk_nonblocking_test.c index 427d03e46eb..85d0981ae9f 100644 --- a/tests/unit/s2n_self_talk_nonblocking_test.c +++ b/tests/unit/s2n_self_talk_nonblocking_test.c @@ -44,8 +44,9 @@ int mock_client(struct s2n_test_io_pair *io_pair, uint8_t *expected_data, uint32 * CI/CD pipelines might never complete */ int should_block = 1; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair underlying io_pair is valid from the moment of fork, + * so the child can start the handshake immediately. The kernel buffers + * writes until the parent is ready to read. */ client_conn = s2n_connection_new(S2N_CLIENT); client_config = s2n_config_new(); @@ -110,8 +111,9 @@ int mock_client_iov(struct s2n_test_io_pair *io_pair, struct iovec *iov, uint32_ uint8_t *buffer = malloc(total_size + iov[0].iov_len); int buffer_offs = 0; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair underlying io_pair is valid from the moment of fork, + * so the child can start the handshake immediately. The kernel buffers + * writes until the parent is ready to read. */ client_conn = s2n_connection_new(S2N_CLIENT); client_config = s2n_config_new(); @@ -336,8 +338,14 @@ int test_send(int use_tls13, int use_iov, int prefer_throughput) EXPECT_TRUE(remaining < data_size); EXPECT_TRUE(remaining > 0); - /* Wait for the child process to read some bytes and block itself*/ - sleep(1); + /* Wait for the child to actually be stopped before sending SIGCONT. + * WUNTRACED causes waitpid to return when the child changes state to + * stopped, not just when it exits. This removes the timing race that + * would otherwise require a sleep() here. */ + int stop_status = 0; + pid_t wait_rc = waitpid(pid, &stop_status, WUNTRACED); + EXPECT_EQUAL(wait_rc, pid); + EXPECT_TRUE(WIFSTOPPED(stop_status)); /* Wake the child process by sending it SIGCONT */ EXPECT_SUCCESS(kill(pid, SIGCONT)); diff --git a/tests/unit/s2n_self_talk_session_id_test.c b/tests/unit/s2n_self_talk_session_id_test.c index dbef1031b01..9621c157027 100644 --- a/tests/unit/s2n_self_talk_session_id_test.c +++ b/tests/unit/s2n_self_talk_session_id_test.c @@ -147,8 +147,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) s2n_blocked_status blocked; int result = 0; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair is valid from fork; the kernel buffers writes until the + * parent reads, so no pre-negotiate sleep is needed. */ /* Initial handshake */ conn = s2n_connection_new(S2N_CLIENT); @@ -208,8 +208,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) s2n_connection_free(conn); - /* Give the server a chance to avoid sigpipe */ - sleep(1); + /* SIGPIPE is inherited as SIG_IGN across fork via s2n_io_pair_init, so + * no pre-cleanup sleep is needed. */ /* Session resumption */ conn = s2n_connection_new(S2N_CLIENT); @@ -241,8 +241,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) s2n_connection_free(conn); - /* Give the server a chance to avoid sigpipe */ - sleep(1); + /* SIGPIPE is inherited as SIG_IGN across fork via s2n_io_pair_init, so + * no pre-cleanup sleep is needed. */ /* Session resumption with bad session state */ conn = s2n_connection_new(S2N_CLIENT); @@ -282,8 +282,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) s2n_connection_free(conn); s2n_config_free(config); - /* Give the server a chance to avoid sigpipe */ - sleep(1); + /* SIGPIPE is inherited as SIG_IGN across fork via s2n_io_pair_init, so + * no pre-cleanup sleep is needed. */ s2n_io_pair_close_one_end(io_pair, S2N_CLIENT); exit(result); diff --git a/tests/unit/s2n_self_talk_tls12_test.c b/tests/unit/s2n_self_talk_tls12_test.c index e3713e30fad..51e25711a89 100644 --- a/tests/unit/s2n_self_talk_tls12_test.c +++ b/tests/unit/s2n_self_talk_tls12_test.c @@ -36,8 +36,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) struct s2n_config *config = NULL; s2n_blocked_status blocked; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair is valid from fork; the kernel buffers writes until the + * parent reads, so no pre-negotiate sleep is needed. */ conn = s2n_connection_new(S2N_CLIENT); config = s2n_config_new(); @@ -89,8 +89,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) s2n_connection_free(conn); s2n_config_free(config); - /* Give the server a chance to a void a sigpipe */ - sleep(1); + /* SIGPIPE is inherited as SIG_IGN across fork via s2n_io_pair_init, so + * no pre-cleanup sleep is needed. */ s2n_io_pair_close_one_end(io_pair, S2N_CLIENT); diff --git a/tests/unit/s2n_self_talk_tls13_test.c b/tests/unit/s2n_self_talk_tls13_test.c index ded3a95c8ae..938f38ddd3a 100644 --- a/tests/unit/s2n_self_talk_tls13_test.c +++ b/tests/unit/s2n_self_talk_tls13_test.c @@ -32,8 +32,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) struct s2n_config *config = NULL; s2n_blocked_status blocked; - /* Give the server a chance to listen */ - sleep(1); + /* The socketpair is valid from fork; the kernel buffers writes until the + * parent reads, so no pre-negotiate sleep is needed. */ conn = s2n_connection_new(S2N_CLIENT); config = s2n_config_new(); @@ -86,8 +86,8 @@ void mock_client(struct s2n_test_io_pair *io_pair) s2n_connection_free(conn); s2n_config_free(config); - /* Give the server a chance to avoid a sigpipe */ - sleep(1); + /* SIGPIPE is inherited as SIG_IGN across fork via s2n_io_pair_init, so + * no pre-cleanup sleep is needed. */ s2n_io_pair_close_one_end(io_pair, S2N_CLIENT);