negotiate_in_use reentrancy flag not cleared on early return error paths

[cpsource]

Summary

In tls/s2n_handshake_io.c lines 1714-1746, the negotiate_in_use reentrancy flag is set at line 1718 but only cleared at line 1744. Multiple early-return error paths between these lines skip the cleanup, permanently deadlocking the connection.

int s2n_negotiate(struct s2n_connection *conn, s2n_blocked_status *blocked)
{
    POSIX_ENSURE_REF(conn);
    POSIX_ENSURE(!conn->negotiate_in_use, S2N_ERR_REENTRANCY);
    conn->negotiate_in_use = true;                                    // line 1718: set

    uint64_t negotiate_start = 0;
    POSIX_GUARD(s2n_default_monotonic_clock(NULL, &negotiate_start)); // line 1723: early return!

    // ... lines 1731-1741 have additional POSIX_GUARD calls that can early return ...

    conn->negotiate_in_use = false;                                   // line 1744: never reached
    return result;
}

Early-return points that skip the flag reset:

• Line 1723: POSIX_GUARD(s2n_default_monotonic_clock(...))
• Line 1731: POSIX_GUARD_RESULT(s2n_connection_dynamic_free_in_buffer(conn))
• Line 1732: POSIX_GUARD_RESULT(s2n_connection_dynamic_free_out_buffer(conn))
• Line 1735: POSIX_GUARD(s2n_default_monotonic_clock(...))
• Line 1740-1741: POSIX_GUARD_RESULT(s2n_event_handshake_populate/send(...))

After any of these fail, all subsequent s2n_negotiate() calls on the same connection will fail with S2N_ERR_REENTRANCY.

Suggested Fix

Use a cleanup pattern to ensure the flag is always cleared:

conn->negotiate_in_use = true;
DEFER_CLEANUP(bool *flag = &conn->negotiate_in_use, s2n_cleanup_bool_flag);

Or restructure so all POSIX_GUARD calls happen inside s2n_negotiate_impl(), before the flag is set.

Impact

Any transient failure in clock retrieval, buffer cleanup, or event reporting permanently deadlocks the connection — no further negotiation is possible without destroying and recreating it.

Git History

• Commit 85649797c (Nov 2021, "Detect nested s2n_negotiate calls Detect nested s2n_negotiate calls #3119") introduced the negotiate_in_use flag with this same pattern. The original commit message notes it was modeled after the mechanism in s2n_send/s2n_recv from commit c8d04e461, which has the same class of bug.

Prior Art Search

• Searched GitHub issues for: negotiate_in_use reentrancy, reentrancy flag error path, negotiate_in_use deadlock, s2n_negotiate reentrancy — no existing reports found.
• Searched git log for reentrancy and negotiate_in_use — only found the original commit above.

Found during code review.

[cpsource]

Analysis and Proposed Fix

Good catch. I've reproduced the issue and developed a fix for all three affected functions.

Problem

The reentrancy flags (negotiate_in_use, send_in_use, recv_in_use) are set at the start of s2n_negotiate(), s2n_sendv_with_offset(), and s2n_recv() — but POSIX_GUARD / POSIX_GUARD_RESULT macros between the flag-set and flag-clear expand to early returns that bypass cleanup. Any transient failure in post-processing permanently deadlocks the connection.

Affected early-return points:

Function	Early-return calls between flag set/clear
s2n_negotiate()	6 (s2n_default_monotonic_clock x2, s2n_connection_dynamic_free_{in,out}_buffer, s2n_event_handshake_{populate,send})
s2n_sendv_with_offset()	2 (s2n_early_data_record_bytes, s2n_connection_dynamic_free_out_buffer)
s2n_recv()	2 (s2n_early_data_record_bytes, s2n_connection_dynamic_free_in_buffer)

Reproduction (PoC)

Triggered the real bug path using LD_PRELOAD to inject a clock_gettime() failure. This makes the second s2n_default_monotonic_clock() call inside s2n_negotiate() fail — after negotiate_in_use is set and after s2n_negotiate_impl() has run — hitting the exact POSIX_GUARD early-return that skips flag cleanup:

$ LD_PRELOAD=./fail_clock.so FAIL_CLOCK_ON=3 ./poc_real_trigger

=== PoC: Issue #5796 - Real trigger via clock_gettime failure ===

1. negotiate_in_use before call: false
2. s2n_negotiate() returned: -1
   Error: S2N_ERR_IO
   negotiate_in_use after call: true

   >>> BUG: negotiate_in_use stuck! Connection is deadlocked. <<<
3. Retry s2n_negotiate() returned: -1
   Error: S2N_ERR_REENTRANCY
   >>> CONFIRMED: S2N_ERR_REENTRANCY - permanent deadlock <<<

=== PoC complete ===

The LD_PRELOAD shim simply wraps clock_gettime() and returns -1 on the Nth call — no internal state manipulation required.

Fix

Replace POSIX_GUARD* macros between the flag-set and flag-clear with explicit checks that goto cleanup, where cleanup always clears the flag before returning. This is the minimal correct change — it preserves all existing behavior on success paths and s2n_errno is set correctly by the failing call before the jump.

Pattern applied to all three functions:

// Before (bug):
conn->negotiate_in_use = true;
POSIX_GUARD(...);              // early return skips cleanup
conn->negotiate_in_use = false;

// After (fix):
conn->negotiate_in_use = true;
int return_value = S2N_FAILURE;
if (some_call() < S2N_SUCCESS) { goto cleanup; }
return_value = result;
cleanup:
    conn->negotiate_in_use = false;
    return return_value;

Files Changed

• tls/s2n_handshake_io.c — s2n_negotiate(): 6 POSIX_GUARD* replaced with explicit checks + goto cleanup
• tls/s2n_send.c — s2n_sendv_with_offset(): 2 POSIX_GUARD_RESULT replaced
• tls/s2n_recv.c — s2n_recv(): 2 POSIX_GUARD_RESULT replaced

Verification

Same clock_gettime failure, same code path — but the flag is now properly cleared:

$ LD_PRELOAD=./fail_clock.so FAIL_CLOCK_ON=3 ./poc_real_trigger

=== PoC: Issue #5796 - Real trigger via clock_gettime failure ===

1. negotiate_in_use before call: false
2. s2n_negotiate() returned: -1
   Error: S2N_ERR_IO
   negotiate_in_use after call: false

   >>> PASS: Flag properly cleared (fix is working) <<<
3. Retry s2n_negotiate() returned: -1
   Error: S2N_ERR_IO
   >>> PASS: No spurious reentrancy error <<<

=== PoC complete ===

Diff

Full diff (click to expand)

diff --git a/tls/s2n_handshake_io.c b/tls/s2n_handshake_io.c
index c5789aa35..991f2aa65 100644
--- a/tls/s2n_handshake_io.c
+++ b/tls/s2n_handshake_io.c
@@ -1717,10 +1717,14 @@ int s2n_negotiate(struct s2n_connection *conn, s2n_blocked_status *blocked)
     POSIX_ENSURE(!conn->negotiate_in_use, S2N_ERR_REENTRANCY);
     conn->negotiate_in_use = true;

+    int return_value = S2N_FAILURE;
+
     /* We use the default monotonic clock so that we can avoid referencing any
      * item on the config until after the client hello callback is invoked. */
     uint64_t negotiate_start = 0;
-    POSIX_GUARD(s2n_default_monotonic_clock(NULL, &negotiate_start));
+    if (s2n_default_monotonic_clock(NULL, &negotiate_start) < S2N_SUCCESS) {
+        goto cleanup;
+    }
     if (conn->handshake_event.handshake_start_ns == 0) {
         conn->handshake_event.handshake_start_ns = negotiate_start;
     }
@@ -1728,19 +1732,32 @@ int s2n_negotiate(struct s2n_connection *conn, s2n_blocked_status *blocked)
     int result = s2n_negotiate_impl(conn, blocked);

     /* finish up sending and receiving */
-    POSIX_GUARD_RESULT(s2n_connection_dynamic_free_in_buffer(conn));
-    POSIX_GUARD_RESULT(s2n_connection_dynamic_free_out_buffer(conn));
+    if (!s2n_result_is_ok(s2n_connection_dynamic_free_in_buffer(conn))) {
+        goto cleanup;
+    }
+    if (!s2n_result_is_ok(s2n_connection_dynamic_free_out_buffer(conn))) {
+        goto cleanup;
+    }

     uint64_t negotiate_end = 0;
-    POSIX_GUARD(s2n_default_monotonic_clock(NULL, &negotiate_end));
+    if (s2n_default_monotonic_clock(NULL, &negotiate_end) < S2N_SUCCESS) {
+        goto cleanup;
+    }
     conn->handshake_event.handshake_time_ns += negotiate_end - negotiate_start;

     if (result == S2N_SUCCESS) {
         conn->handshake_event.handshake_end_ns = negotiate_end;
-        POSIX_GUARD_RESULT(s2n_event_handshake_populate(conn, &conn->handshake_event));
-        POSIX_GUARD_RESULT(s2n_event_handshake_send(conn, &conn->handshake_event));
+        if (!s2n_result_is_ok(s2n_event_handshake_populate(conn, &conn->handshake_event))) {
+            goto cleanup;
+        }
+        if (!s2n_result_is_ok(s2n_event_handshake_send(conn, &conn->handshake_event))) {
+            goto cleanup;
+        }
     }

+    return_value = result;
+
+cleanup:
     conn->negotiate_in_use = false;
-    return result;
+    return return_value;
 }

diff --git a/tls/s2n_send.c b/tls/s2n_send.c
index 042a3e8d5..d22f3d2d8 100644
--- a/tls/s2n_send.c
+++ b/tls/s2n_send.c
@@ -252,13 +252,22 @@ ssize_t s2n_sendv_with_offset(...)
     POSIX_ENSURE(!conn->send_in_use, S2N_ERR_REENTRANCY);
     conn->send_in_use = true;

+    ssize_t return_value = S2N_FAILURE;
+
     ssize_t result = s2n_sendv_with_offset_impl(conn, bufs, count, offs, blocked);
-    POSIX_GUARD_RESULT(s2n_early_data_record_bytes(conn, result));
+    if (!s2n_result_is_ok(s2n_early_data_record_bytes(conn, result))) {
+        goto cleanup;
+    }

-    POSIX_GUARD_RESULT(s2n_connection_dynamic_free_out_buffer(conn));
+    if (!s2n_result_is_ok(s2n_connection_dynamic_free_out_buffer(conn))) {
+        goto cleanup;
+    }

+    return_value = result;
+
+cleanup:
     conn->send_in_use = false;
-    return result;
+    return return_value;
 }

diff --git a/tls/s2n_recv.c b/tls/s2n_recv.c
index 14e1d52a4..7b775362e 100644
--- a/tls/s2n_recv.c
+++ b/tls/s2n_recv.c
@@ -296,14 +296,23 @@ ssize_t s2n_recv(...)
     POSIX_ENSURE(!conn->recv_in_use, S2N_ERR_REENTRANCY);
     conn->recv_in_use = true;

+    ssize_t return_value = S2N_FAILURE;
+
     ssize_t result = s2n_recv_impl(conn, buf, size, blocked);
-    POSIX_GUARD_RESULT(s2n_early_data_record_bytes(conn, result));
+    if (!s2n_result_is_ok(s2n_early_data_record_bytes(conn, result))) {
+        goto cleanup;
+    }

     /* finish the recv call */
-    POSIX_GUARD_RESULT(s2n_connection_dynamic_free_in_buffer(conn));
+    if (!s2n_result_is_ok(s2n_connection_dynamic_free_in_buffer(conn))) {
+        goto cleanup;
+    }
+
+    return_value = result;

+cleanup:
     conn->recv_in_use = false;
-    return result;
+    return return_value;
 }

[cpsource]

s2n-tls.tar.gz

All ctests passed with patch: 279/279 tests passed, 0 failed. Full test suite clean in ~224 seconds.

Attached is test code.