Fix TSAN s2n_shutdown failures

CMakeLists.txt

@@ -221,11 +221,6 @@ if(S2N_UNSAFE_FUZZING_MODE)
     target_compile_options(${PROJECT_NAME} PRIVATE -fsanitize-coverage=trace-pc-guard -fsanitize=address,undefined,leak -fuse-ld=gold -DS2N_ADDRESS_SANITIZER=1)
 endif()
 
-if(TSAN)
-    target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
-    target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
-endif()
-
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")
 
 if (NOT $ENV{S2N_LIBCRYPTO} MATCHES "awslc")
@@ -357,6 +352,13 @@ if (NOT S2N_EXECINFO_AVAILABLE)
 endif()
 feature_probe_result(S2N_STACKTRACE ${S2N_STACKTRACE})
 
+if(TSAN)
+    target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
+    target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
+    # For now, only enable real atomics for TSAN
+    feature_probe_result(S2N_ATOMIC_ENABLED ${S2N_ATOMIC_SUPPORTED})
+endif()
+
 set(S2N_KYBER512R3_AVX2_BMI2 FALSE)
 if(NOT S2N_NO_PQ_ASM)
     # Kyber Round-3 code has several different optimizations which require
@@ -510,7 +512,11 @@ if (BUILD_TESTING)
             message(FATAL_ERROR "TSAN suppression file ${TSAN_SUPPRESSIONS_FILE} missing")
         endif()
         set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} S2N_ADDRESS_SANITIZER=1)
-        set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} TSAN_OPTIONS=suppressions=${TSAN_SUPPRESSIONS_FILE})
+        set(TSAN_OPTIONS suppressions=${TSAN_SUPPRESSIONS_FILE})
+        if(DEFINED ENV{TSAN_OPTIONS})
+            set(TSAN_OPTIONS "${TSAN_OPTIONS} $ENV{TSAN_OPTIONS}")
+        endif()
+        set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} TSAN_OPTIONS=${TSAN_OPTIONS})
     endif()
     message(STATUS "Running tests with environment: ${UNIT_TEST_ENVS}")
 

error/s2n_errno.c

@@ -290,6 +290,7 @@ static const char *no_such_error = "Internal s2n error";
     ERR_ENTRY(S2N_ERR_INTERNAL_LIBCRYPTO_ERROR, "An internal error has occurred in the libcrypto API") \
     ERR_ENTRY(S2N_ERR_NO_RENEGOTIATION, "Only secure, server-initiated renegotiation is supported") \
     ERR_ENTRY(S2N_ERR_APP_DATA_BLOCKED, "Blocked on application data during handshake") \
+    ERR_ENTRY(S2N_ERR_ATOMIC, "Atomic operations in this environment would require locking") \
     /* clang-format on */
 
 #define ERR_STR_CASE(ERR, str) \

error/s2n_errno.h

@@ -306,6 +306,7 @@ typedef enum {
     S2N_ERR_SECRET_SCHEDULE_STATE,
     S2N_ERR_CERT_OWNERSHIP,
     S2N_ERR_INTERNAL_LIBCRYPTO_ERROR,
+    S2N_ERR_ATOMIC,
     S2N_ERR_T_USAGE_END,
 } s2n_error;
 

tests/features/S2N_ATOMIC_SUPPORTED.c

@@ -0,0 +1,23 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include <signal.h>
+
+int main() {
+    sig_atomic_t atomic = 0;
+    __atomic_test_and_set(&atomic, __ATOMIC_RELAXED);
+    __atomic_clear(&atomic, __ATOMIC_RELAXED);
+    return 0;
+}

tests/features/S2N_ATOMIC_SUPPORTED.flags

@@ -0,0 +1 @@
+-Werror

tls/s2n_alerts.c

@@ -23,6 +23,7 @@
 #include "tls/s2n_record.h"
 #include "tls/s2n_resume.h"
 #include "tls/s2n_tls_parameters.h"
+#include "utils/s2n_atomic.h"
 #include "utils/s2n_blob.h"
 #include "utils/s2n_safety.h"
 
@@ -203,8 +204,8 @@ int s2n_process_alert_fragment(struct s2n_connection *conn)
         if (s2n_stuffer_data_available(&conn->alert_in) == 2) {
             /* Close notifications are handled as shutdowns */
             if (conn->alert_in_data[1] == S2N_TLS_ALERT_CLOSE_NOTIFY) {
-                conn->read_closed = 1;
-                conn->close_notify_received = true;
+                s2n_atomic_set(&conn->read_closed);
+                s2n_atomic_set(&conn->close_notify_received);
                 return 0;
             }
 
@@ -221,6 +222,7 @@ int s2n_process_alert_fragment(struct s2n_connection *conn)
 
             /* All other alerts are treated as fatal errors */
             POSIX_GUARD_RESULT(s2n_connection_set_closed(conn));
+            s2n_atomic_set(&conn->error_alert_received);
             POSIX_BAIL(S2N_ERR_ALERT);
         }
     }

tls/s2n_connection.c

@@ -43,6 +43,7 @@
 #include "tls/s2n_security_policies.h"
 #include "tls/s2n_tls.h"
 #include "tls/s2n_tls_parameters.h"
+#include "utils/s2n_atomic.h"
 #include "utils/s2n_blob.h"
 #include "utils/s2n_compiler.h"
 #include "utils/s2n_mem.h"
@@ -1168,8 +1169,8 @@ S2N_CLEANUP_RESULT s2n_connection_apply_error_blinding(struct s2n_connection **c
 S2N_RESULT s2n_connection_set_closed(struct s2n_connection *conn)
 {
     RESULT_ENSURE_REF(conn);
-    conn->read_closed = 1;
-    conn->write_closed = 1;
+    s2n_atomic_set(&conn->read_closed);
+    s2n_atomic_set(&conn->write_closed);
     return S2N_RESULT_OK;
 }
 
@@ -1547,7 +1548,9 @@ bool s2n_connection_check_io_status(struct s2n_connection *conn, s2n_io_status s
         return false;
     }
 
-    const bool is_full_duplex = !conn->read_closed && !conn->write_closed;
+    bool read_closed = s2n_atomic_check(&conn->read_closed);
+    bool write_closed = s2n_atomic_check(&conn->write_closed);
+    bool full_duplex = !read_closed && !write_closed;
 
     /*
      *= https://tools.ietf.org/rfc/rfc8446#section-6.1
@@ -1561,21 +1564,21 @@ bool s2n_connection_check_io_status(struct s2n_connection *conn, s2n_io_status s
             case S2N_IO_WRITABLE:
             case S2N_IO_READABLE:
             case S2N_IO_FULL_DUPLEX:
-                return is_full_duplex;
+                return full_duplex;
             case S2N_IO_CLOSED:
-                return !is_full_duplex;
+                return !full_duplex;
         }
     }
 
     switch (status) {
         case S2N_IO_WRITABLE:
-            return !conn->write_closed;
+            return !write_closed;
         case S2N_IO_READABLE:
-            return !conn->read_closed;
+            return !read_closed;
         case S2N_IO_FULL_DUPLEX:
-            return is_full_duplex;
+            return full_duplex;
         case S2N_IO_CLOSED:
-            return conn->read_closed && conn->write_closed;
+            return read_closed && write_closed;
     }
 
     return false;

tls/s2n_connection.h

@@ -39,6 +39,7 @@
 #include "tls/s2n_security_policies.h"
 #include "tls/s2n_tls_parameters.h"
 #include "tls/s2n_x509_validator.h"
+#include "utils/s2n_atomic.h"
 #include "utils/s2n_mem.h"
 #include "utils/s2n_timer.h"
 
@@ -110,9 +111,6 @@ struct s2n_connection {
      * even when setting a new config. */
     unsigned psk_mode_overridden : 1;
 
-    /* Have we received a close notify alert from the peer. */
-    unsigned close_notify_received : 1;
-
     /* Connection negotiated an EMS */
     unsigned ems_negotiated : 1;
 
@@ -272,6 +270,10 @@ struct s2n_connection {
     uint8_t reader_warning_out;
     bool alert_sent;
 
+    /* Receiving error or close_notify alerts changes the behavior of s2n_shutdown_send */
+    s2n_atomic error_alert_received;
+    s2n_atomic close_notify_received;
+
     /* Our handshake state machine */
     struct s2n_handshake handshake;
 
@@ -308,13 +310,11 @@ struct s2n_connection {
     uint64_t wire_bytes_out;
     uint64_t early_data_bytes;
 
-    /* Is the connection open or closed?
-     *
-     * We use C's only atomic type as an error requires shutting down both read
-     * and write, so both the reader and writer threads may access both fields.
+    /* Either the reader or the writer can trigger both sides of the connection
+     * to close in response to a fatal error.
      */
-    sig_atomic_t read_closed;
-    sig_atomic_t write_closed;
+    s2n_atomic read_closed;
+    s2n_atomic write_closed;
 
     /* TLS extension data */
     char server_name[S2N_MAX_SERVER_NAME + 1];

tls/s2n_recv.c

@@ -42,7 +42,7 @@ S2N_RESULT s2n_read_in_bytes(struct s2n_connection *conn, struct s2n_stuffer *ou
         errno = 0;
         int r = s2n_connection_recv_stuffer(output, conn, remaining);
         if (r == 0) {
-            conn->read_closed = 1;
+            s2n_atomic_set(&conn->read_closed);
             RESULT_BAIL(S2N_ERR_CLOSED);
         } else if (r < 0) {
             if (errno == EWOULDBLOCK || errno == EAGAIN) {
@@ -127,7 +127,7 @@ ssize_t s2n_recv_impl(struct s2n_connection *conn, void *buf, ssize_t size, s2n_
          *# closed, the TLS implementation MUST receive a "close_notify" alert
          *# before indicating end-of-data to the application layer.
          */
-        POSIX_ENSURE(conn->close_notify_received, S2N_ERR_CLOSED);
+        POSIX_ENSURE(s2n_atomic_check(&conn->close_notify_received), S2N_ERR_CLOSED);
         *blocked = S2N_NOT_BLOCKED;
         return 0;
     }

tls/s2n_shutdown.c

@@ -17,29 +17,13 @@
 #include "tls/s2n_alerts.h"
 #include "tls/s2n_connection.h"
 #include "tls/s2n_tls.h"
+#include "utils/s2n_atomic.h"
 #include "utils/s2n_safety.h"
 
-static bool s2n_error_alert_received(struct s2n_connection *conn)
-{
-    /* We don't check s2n_connection_get_alert() or s2n_stuffer_data_available()
-     * because of https://github.com/aws/s2n-tls/issues/3933.
-     * We need to check if the stuffer contains an alert, regardless of its
-     * read state.
-     */
-    if (conn->alert_in.write_cursor == 0) {
-        return false;
-    }
-    /* Verify that the stuffer doesn't just contain a close_notify alert */
-    if (conn->close_notify_received) {
-        return false;
-    }
-    return true;
-}
-
 static bool s2n_shutdown_expect_close_notify(struct s2n_connection *conn)
 {
     /* No close_notify expected if we already received an error instead */
-    if (s2n_error_alert_received(conn)) {
+    if (s2n_atomic_check(&conn->error_alert_received)) {
         return false;
     }
 
@@ -83,11 +67,11 @@ int s2n_shutdown_send(struct s2n_connection *conn, s2n_blocked_status *blocked)
     }
 
     /* Flush any outstanding data */
-    conn->write_closed = 1;
+    s2n_atomic_set(&conn->write_closed);
     POSIX_GUARD(s2n_flush(conn, blocked));
 
     /* For a connection closed due to receiving an alert, we don't send anything. */
-    if (s2n_error_alert_received(conn)) {
+    if (s2n_atomic_check(&conn->error_alert_received)) {
         return S2N_SUCCESS;
     }
 
@@ -136,7 +120,7 @@ int s2n_shutdown(struct s2n_connection *conn, s2n_blocked_status *blocked)
     uint8_t record_type = 0;
     int isSSLv2 = false;
     *blocked = S2N_BLOCKED_ON_READ;
-    while (!conn->close_notify_received) {
+    while (!s2n_atomic_check(&conn->close_notify_received)) {
         POSIX_GUARD(s2n_read_full_record(conn, &record_type, &isSSLv2));
         POSIX_ENSURE(!isSSLv2, S2N_ERR_BAD_MESSAGE);
         if (record_type == TLS_ALERT) {

utils/s2n_atomic.c

@@ -0,0 +1,58 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include "utils/s2n_atomic.h"
+
+#include "utils/s2n_safety.h"
+
+const s2n_atomic set_val = 1;
+const s2n_atomic clear_val = 0;
+
+S2N_RESULT s2n_atomic_init()
+{
+#if S2N_ATOMIC_ENABLED
+    RESULT_ENSURE(__atomic_always_lock_free(sizeof(s2n_atomic), NULL), S2N_ERR_ATOMIC);
+#endif
+    return S2N_RESULT_OK;
+}
+
+void s2n_atomic_set(s2n_atomic *var)
+{
+#if S2N_ATOMIC_ENABLED
+    __atomic_store(var, &set_val, __ATOMIC_RELAXED);
+#else
+    *var = 1;
+#endif
+}
+
+void s2n_atomic_clear(s2n_atomic *var)
+{
+#if S2N_ATOMIC_ENABLED
+    __atomic_store(var, &clear_val, __ATOMIC_RELAXED);
+#else
+    *var = 0;
+#endif
+}
+
+bool s2n_atomic_check(s2n_atomic *var)
+{
+#if S2N_ATOMIC_ENABLED
+    s2n_atomic result = 0;
+    __atomic_load(var, &result, __ATOMIC_RELAXED);
+    return result;
+#else
+    return *var;
+#endif
+}

utils/s2n_atomic.h

@@ -0,0 +1,35 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+#pragma once
+
+#include <signal.h>
+
+#include "utils/s2n_result.h"
+
+/* s2n-tls allows s2n_send and s2n_recv to be called concurrently.
+ * There are a handful of values that both methods need to access.
+ * However, C99 has no concept of concurrency, so provides no atomic data types.
+ *
+ * Traditionally, s2n-tls uses sig_atomic_t for its weak guarantee of atomicity for interrupts.
+ */
+typedef sig_atomic_t s2n_atomic;
+
+/* These methods use compiler atomic built-ins if available and lock-free, but otherwise
+ * rely on setting / clearing a small value generally being atomic in practice.
+ */
+S2N_RESULT s2n_atomic_init();
+void s2n_atomic_set(s2n_atomic *var);
+void s2n_atomic_clear(s2n_atomic *var);
+bool s2n_atomic_check(s2n_atomic *var);

utils/s2n_init.c

@@ -79,6 +79,7 @@ int s2n_init(void)
     POSIX_GUARD(s2n_extension_type_init());
     POSIX_GUARD_RESULT(s2n_pq_init());
     POSIX_GUARD_RESULT(s2n_tls13_empty_transcripts_init());
+    POSIX_GUARD_RESULT(s2n_atomic_init());
 
     if (atexit_cleanup) {
         POSIX_ENSURE_OK(atexit(s2n_cleanup_atexit), S2N_ERR_ATEXIT);