From d0322c025b4098646cda3b339f65e8877956cd25 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 14 Jun 2023 10:46:56 -0700 Subject: [PATCH 1/6] Fix TSAN s2n_shutdown failures --- CMakeLists.txt | 18 ++++--- error/s2n_errno.c | 1 + error/s2n_errno.h | 1 + tests/features/S2N_ATOMIC_SUPPORTED.c | 23 +++++++++ tests/features/S2N_ATOMIC_SUPPORTED.flags | 1 + tls/s2n_alerts.c | 6 ++- tls/s2n_connection.c | 21 ++++---- tls/s2n_connection.h | 18 +++---- tls/s2n_recv.c | 4 +- tls/s2n_shutdown.c | 26 ++-------- utils/s2n_atomic.c | 58 +++++++++++++++++++++++ utils/s2n_atomic.h | 35 ++++++++++++++ utils/s2n_init.c | 1 + 13 files changed, 164 insertions(+), 49 deletions(-) create mode 100644 tests/features/S2N_ATOMIC_SUPPORTED.c create mode 100644 tests/features/S2N_ATOMIC_SUPPORTED.flags create mode 100644 utils/s2n_atomic.c create mode 100644 utils/s2n_atomic.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 4e4ad337f04..30ac35fbff4 100644 --- a/CMakeLists.txt +++ b/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}") diff --git a/error/s2n_errno.c b/error/s2n_errno.c index ecb9dc86f7f..c958e9f3065 100644 --- a/error/s2n_errno.c +++ b/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) \ diff --git a/error/s2n_errno.h b/error/s2n_errno.h index 151159f59f0..dd5b841b1c0 100644 --- a/error/s2n_errno.h +++ b/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; diff --git a/tests/features/S2N_ATOMIC_SUPPORTED.c b/tests/features/S2N_ATOMIC_SUPPORTED.c new file mode 100644 index 00000000000..d371aaee3a4 --- /dev/null +++ b/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 + +int main() { + sig_atomic_t atomic = 0; + __atomic_test_and_set(&atomic, __ATOMIC_RELAXED); + __atomic_clear(&atomic, __ATOMIC_RELAXED); + return 0; +} diff --git a/tests/features/S2N_ATOMIC_SUPPORTED.flags b/tests/features/S2N_ATOMIC_SUPPORTED.flags new file mode 100644 index 00000000000..2f41be663b2 --- /dev/null +++ b/tests/features/S2N_ATOMIC_SUPPORTED.flags @@ -0,0 +1 @@ +-Werror diff --git a/tls/s2n_alerts.c b/tls/s2n_alerts.c index db53ac148be..0ff898638b1 100644 --- a/tls/s2n_alerts.c +++ b/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); } } diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index 285ecd948ce..4d14c8daa75 100644 --- a/tls/s2n_connection.c +++ b/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; diff --git a/tls/s2n_connection.h b/tls/s2n_connection.h index 01563e1c750..3363fae5cc0 100644 --- a/tls/s2n_connection.h +++ b/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]; diff --git a/tls/s2n_recv.c b/tls/s2n_recv.c index 0f6256844f1..7fd4d9a8ec1 100644 --- a/tls/s2n_recv.c +++ b/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; } diff --git a/tls/s2n_shutdown.c b/tls/s2n_shutdown.c index 9072f6a1eb2..24ad7b2ead8 100644 --- a/tls/s2n_shutdown.c +++ b/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) { diff --git a/utils/s2n_atomic.c b/utils/s2n_atomic.c new file mode 100644 index 00000000000..edbe486c829 --- /dev/null +++ b/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 +} diff --git a/utils/s2n_atomic.h b/utils/s2n_atomic.h new file mode 100644 index 00000000000..3b77df2b723 --- /dev/null +++ b/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 + +#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); diff --git a/utils/s2n_init.c b/utils/s2n_init.c index 6f7efe19450..cffc8d00c4e 100644 --- a/utils/s2n_init.c +++ b/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); From 6b7cadd9403033fb55f55d5e39b89b7258b476c9 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 14 Jun 2023 16:50:38 -0700 Subject: [PATCH 2/6] PR comments: renames + move to struct --- tests/unit/s2n_alerts_test.c | 8 +++--- tests/unit/s2n_connection_test.c | 32 +++++++++------------- tests/unit/s2n_early_data_io_api_test.c | 4 +-- tests/unit/s2n_shutdown_test.c | 36 ++++++++++++------------- tls/s2n_alerts.c | 6 ++--- tls/s2n_connection.c | 8 +++--- tls/s2n_connection.h | 8 +++--- tls/s2n_recv.c | 4 +-- tls/s2n_shutdown.c | 8 +++--- utils/s2n_atomic.c | 31 ++++++++------------- utils/s2n_atomic.h | 18 ++++++++----- 11 files changed, 76 insertions(+), 87 deletions(-) diff --git a/tests/unit/s2n_alerts_test.c b/tests/unit/s2n_alerts_test.c index 2d4d87b583d..e930c8b551a 100644 --- a/tests/unit/s2n_alerts_test.c +++ b/tests/unit/s2n_alerts_test.c @@ -106,7 +106,7 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT)); /* Verify state prior to alert */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); /* Write and process the alert */ EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->in, not_close_notify_alert, sizeof(not_close_notify_alert))); @@ -115,7 +115,7 @@ int main(int argc, char **argv) EXPECT_FAILURE_WITH_ERRNO(s2n_process_alert_fragment(conn), S2N_ERR_ALERT); /* Verify state after alert */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_SUCCESS(s2n_connection_free(conn)); } @@ -126,14 +126,14 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT)); /* Verify state prior to alert */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); /* Write and process the alert */ EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->in, close_notify_alert, sizeof(close_notify_alert))); EXPECT_SUCCESS(s2n_process_alert_fragment(conn)); /* Verify state after alert */ - EXPECT_TRUE(conn->close_notify_received); + EXPECT_TRUE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_SUCCESS(s2n_connection_free(conn)); } diff --git a/tests/unit/s2n_connection_test.c b/tests/unit/s2n_connection_test.c index 73dc90c1318..d982890ad08 100644 --- a/tests/unit/s2n_connection_test.c +++ b/tests/unit/s2n_connection_test.c @@ -795,14 +795,6 @@ int main(int argc, char **argv) EXPECT_FALSE(s2n_connection_check_io_status(NULL, S2N_IO_CLOSED)); EXPECT_FALSE(s2n_connection_check_io_status(NULL, 10)); EXPECT_FALSE(s2n_connection_check_io_status(conn, 10)); - - EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); - conn->write_closed = 10; - EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); - - EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); - conn->read_closed = 10; - EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); } /* TLS1.2 */ @@ -819,24 +811,24 @@ int main(int argc, char **argv) EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); /* Close write */ - conn->write_closed = 1; + s2n_atomic_store(&conn->write_closed, true); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - conn->write_closed = 0; + s2n_atomic_store(&conn->write_closed, false); /* Close read */ - conn->read_closed = 1; + s2n_atomic_store(&conn->read_closed, true); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - conn->read_closed = 0; + s2n_atomic_store(&conn->read_closed, false); /* Close both */ - conn->read_closed = 1; - conn->write_closed = 1; + s2n_atomic_store(&conn->read_closed, true); + s2n_atomic_store(&conn->write_closed, true); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -857,24 +849,24 @@ int main(int argc, char **argv) EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); /* Close write */ - conn->write_closed = 1; + s2n_atomic_store(&conn->write_closed, true); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - conn->write_closed = 0; + s2n_atomic_store(&conn->write_closed, false); /* Close read */ - conn->read_closed = 1; + s2n_atomic_store(&conn->read_closed, true); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - conn->read_closed = 0; + s2n_atomic_store(&conn->read_closed, false); /* Close both */ - conn->read_closed = 1; - conn->write_closed = 1; + s2n_atomic_store(&conn->read_closed, true); + s2n_atomic_store(&conn->write_closed, true); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); diff --git a/tests/unit/s2n_early_data_io_api_test.c b/tests/unit/s2n_early_data_io_api_test.c index c04f777c96b..8b05a0b83a5 100644 --- a/tests/unit/s2n_early_data_io_api_test.c +++ b/tests/unit/s2n_early_data_io_api_test.c @@ -373,8 +373,8 @@ int main(int argc, char **argv) /* Pretend we didn't test the above error condition. * The S2N_ERR_BAD_MESSAGE error triggered S2N to close the connection. */ - server_conn->write_closed = false; - client_conn->write_closed = false; + s2n_atomic_store(&server_conn->write_closed, false); + s2n_atomic_store(&client_conn->write_closed, false); /* Read the remaining early data properly */ EXPECT_SUCCESS(s2n_recv_early_data(server_conn, actual_payload, sizeof(actual_payload), diff --git a/tests/unit/s2n_shutdown_test.c b/tests/unit/s2n_shutdown_test.c index 7a01af33b72..00d9b37a4cf 100644 --- a/tests/unit/s2n_shutdown_test.c +++ b/tests/unit/s2n_shutdown_test.c @@ -66,7 +66,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -78,7 +78,7 @@ int main(int argc, char **argv) /* Verify state after shutdown attempt */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); @@ -109,7 +109,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -123,7 +123,7 @@ int main(int argc, char **argv) /* Verify state after shutdown attempt */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); @@ -146,7 +146,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -173,7 +173,7 @@ int main(int argc, char **argv) /* Verify state after shutdown attempt */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); @@ -195,7 +195,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_FALSE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -204,7 +204,7 @@ int main(int argc, char **argv) /* Verify state after shutdown */ EXPECT_FALSE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); /* Fully closed: we don't worry about truncating data */ @@ -225,7 +225,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_connection_set_io_stuffers(&input, &output, conn)); /* Verify state prior to alert */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -234,7 +234,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(blocked, S2N_BLOCKED_ON_READ); /* Verify state after shutdown attempt */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); /* Half-close: only write closed */ @@ -257,7 +257,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_connection_set_io_stuffers(&input, &output, conn)); /* Verify state prior to alert */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -266,7 +266,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_process_alert_fragment(conn)); /* Verify state after alert */ - EXPECT_TRUE(conn->close_notify_received); + EXPECT_TRUE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); @@ -276,7 +276,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(blocked, S2N_NOT_BLOCKED); /* Verify state after shutdown attempt */ - EXPECT_TRUE(conn->close_notify_received); + EXPECT_TRUE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); }; @@ -309,7 +309,7 @@ int main(int argc, char **argv) EXPECT_FAILURE_WITH_ERRNO(s2n_shutdown(conn, &blocked), S2N_ERR_ALERT); /* Verify state after shutdown attempt */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); EXPECT_EQUAL(s2n_stuffer_data_available(&output), alert_record_size); @@ -317,7 +317,7 @@ int main(int argc, char **argv) /* Future calls are no-ops */ for (size_t i = 0; i < 5; i++) { EXPECT_SUCCESS(s2n_shutdown(conn, &blocked)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); } }; @@ -410,7 +410,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(blocked, S2N_NOT_BLOCKED); /* Verify state after shutdown attempt */ - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); }; @@ -586,7 +586,7 @@ int main(int argc, char **argv) /* Full close is no-op */ EXPECT_SUCCESS(s2n_shutdown(conn, &blocked)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); }; /* Test: Half close, peer alert, then full close */ @@ -621,7 +621,7 @@ int main(int argc, char **argv) /* Full close is no-op */ EXPECT_SUCCESS(s2n_shutdown(conn, &blocked)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - EXPECT_FALSE(conn->close_notify_received); + EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); }; } diff --git a/tls/s2n_alerts.c b/tls/s2n_alerts.c index 0ff898638b1..9d9cffccee3 100644 --- a/tls/s2n_alerts.c +++ b/tls/s2n_alerts.c @@ -204,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) { - s2n_atomic_set(&conn->read_closed); - s2n_atomic_set(&conn->close_notify_received); + s2n_atomic_store(&conn->read_closed, true); + s2n_atomic_store(&conn->close_notify_received, true); return 0; } @@ -222,7 +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); + s2n_atomic_store(&conn->error_alert_received, true); POSIX_BAIL(S2N_ERR_ALERT); } } diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index 4d14c8daa75..b2e27d4b314 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -1169,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); - s2n_atomic_set(&conn->read_closed); - s2n_atomic_set(&conn->write_closed); + s2n_atomic_store(&conn->read_closed, true); + s2n_atomic_store(&conn->write_closed, true); return S2N_RESULT_OK; } @@ -1548,8 +1548,8 @@ bool s2n_connection_check_io_status(struct s2n_connection *conn, s2n_io_status s return false; } - bool read_closed = s2n_atomic_check(&conn->read_closed); - bool write_closed = s2n_atomic_check(&conn->write_closed); + bool read_closed = s2n_atomic_load(&conn->read_closed); + bool write_closed = s2n_atomic_load(&conn->write_closed); bool full_duplex = !read_closed && !write_closed; /* diff --git a/tls/s2n_connection.h b/tls/s2n_connection.h index 3363fae5cc0..940ef5abcac 100644 --- a/tls/s2n_connection.h +++ b/tls/s2n_connection.h @@ -271,8 +271,8 @@ struct s2n_connection { 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; + s2n_atomic_bool error_alert_received; + s2n_atomic_bool close_notify_received; /* Our handshake state machine */ struct s2n_handshake handshake; @@ -313,8 +313,8 @@ struct s2n_connection { /* Either the reader or the writer can trigger both sides of the connection * to close in response to a fatal error. */ - s2n_atomic read_closed; - s2n_atomic write_closed; + s2n_atomic_bool read_closed; + s2n_atomic_bool write_closed; /* TLS extension data */ char server_name[S2N_MAX_SERVER_NAME + 1]; diff --git a/tls/s2n_recv.c b/tls/s2n_recv.c index 7fd4d9a8ec1..6a37c8bcb73 100644 --- a/tls/s2n_recv.c +++ b/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) { - s2n_atomic_set(&conn->read_closed); + s2n_atomic_store(&conn->read_closed, true); 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(s2n_atomic_check(&conn->close_notify_received), S2N_ERR_CLOSED); + POSIX_ENSURE(s2n_atomic_load(&conn->close_notify_received), S2N_ERR_CLOSED); *blocked = S2N_NOT_BLOCKED; return 0; } diff --git a/tls/s2n_shutdown.c b/tls/s2n_shutdown.c index 24ad7b2ead8..a54c8b3d9cb 100644 --- a/tls/s2n_shutdown.c +++ b/tls/s2n_shutdown.c @@ -23,7 +23,7 @@ static bool s2n_shutdown_expect_close_notify(struct s2n_connection *conn) { /* No close_notify expected if we already received an error instead */ - if (s2n_atomic_check(&conn->error_alert_received)) { + if (s2n_atomic_load(&conn->error_alert_received)) { return false; } @@ -67,11 +67,11 @@ int s2n_shutdown_send(struct s2n_connection *conn, s2n_blocked_status *blocked) } /* Flush any outstanding data */ - s2n_atomic_set(&conn->write_closed); + s2n_atomic_store(&conn->write_closed, true); POSIX_GUARD(s2n_flush(conn, blocked)); /* For a connection closed due to receiving an alert, we don't send anything. */ - if (s2n_atomic_check(&conn->error_alert_received)) { + if (s2n_atomic_load(&conn->error_alert_received)) { return S2N_SUCCESS; } @@ -120,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 (!s2n_atomic_check(&conn->close_notify_received)) { + while (!s2n_atomic_load(&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) { diff --git a/utils/s2n_atomic.c b/utils/s2n_atomic.c index edbe486c829..3fa931b7b6e 100644 --- a/utils/s2n_atomic.c +++ b/utils/s2n_atomic.c @@ -15,44 +15,35 @@ #include "utils/s2n_atomic.h" -#include "utils/s2n_safety.h" +#include -const s2n_atomic set_val = 1; -const s2n_atomic clear_val = 0; +#include "utils/s2n_safety.h" S2N_RESULT s2n_atomic_init() { #if S2N_ATOMIC_ENABLED - RESULT_ENSURE(__atomic_always_lock_free(sizeof(s2n_atomic), NULL), S2N_ERR_ATOMIC); + RESULT_ENSURE(__atomic_always_lock_free(sizeof(s2n_atomic_bool), 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) +void s2n_atomic_store(s2n_atomic_bool *var, bool val) { #if S2N_ATOMIC_ENABLED - __atomic_store(var, &clear_val, __ATOMIC_RELAXED); + sig_atomic_t input = val; + __atomic_store(&var->val, &input, __ATOMIC_RELAXED); #else - *var = 0; + var->val = val; #endif } -bool s2n_atomic_check(s2n_atomic *var) +bool s2n_atomic_load(s2n_atomic_bool *var) { #if S2N_ATOMIC_ENABLED - s2n_atomic result = 0; - __atomic_load(var, &result, __ATOMIC_RELAXED); + sig_atomic_t result = 0; + __atomic_load(&var->val, &result, __ATOMIC_RELAXED); return result; #else - return *var; + return var->val; #endif } diff --git a/utils/s2n_atomic.h b/utils/s2n_atomic.h index 3b77df2b723..9720e42e972 100644 --- a/utils/s2n_atomic.h +++ b/utils/s2n_atomic.h @@ -21,15 +21,21 @@ /* 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; + +/* Wrap the underlying value in a struct to encourage developers to only use + * the provided atomic methods. + */ +typedef struct { + /* Traditionally, s2n-tls uses sig_atomic_t for its weak guarantee of + * atomicity for interrupts. + */ + sig_atomic_t val; +} s2n_atomic_bool; /* 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); +void s2n_atomic_store(s2n_atomic_bool *var, bool value); +bool s2n_atomic_load(s2n_atomic_bool *var); From 255447e8863b44923eeeab785a3e54c246d888fd Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Thu, 15 Jun 2023 00:01:23 +0000 Subject: [PATCH 3/6] PR comments: rework compiler flags --- CMakeLists.txt | 12 +++++------- utils/s2n_atomic.c | 6 +++--- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 30ac35fbff4..bc8e6554842 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -221,6 +221,11 @@ 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 -DS2N_THREAD_SANITIZER=1) + 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") @@ -352,13 +357,6 @@ 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 diff --git a/utils/s2n_atomic.c b/utils/s2n_atomic.c index 3fa931b7b6e..8496bd40732 100644 --- a/utils/s2n_atomic.c +++ b/utils/s2n_atomic.c @@ -21,7 +21,7 @@ S2N_RESULT s2n_atomic_init() { -#if S2N_ATOMIC_ENABLED +#if S2N_ATOMIC_SUPPORTED && S2N_THREAD_SANITIZER RESULT_ENSURE(__atomic_always_lock_free(sizeof(s2n_atomic_bool), NULL), S2N_ERR_ATOMIC); #endif return S2N_RESULT_OK; @@ -29,7 +29,7 @@ S2N_RESULT s2n_atomic_init() void s2n_atomic_store(s2n_atomic_bool *var, bool val) { -#if S2N_ATOMIC_ENABLED +#if S2N_ATOMIC_SUPPORTED && S2N_THREAD_SANITIZER sig_atomic_t input = val; __atomic_store(&var->val, &input, __ATOMIC_RELAXED); #else @@ -39,7 +39,7 @@ void s2n_atomic_store(s2n_atomic_bool *var, bool val) bool s2n_atomic_load(s2n_atomic_bool *var) { -#if S2N_ATOMIC_ENABLED +#if S2N_ATOMIC_SUPPORTED && S2N_THREAD_SANITIZER sig_atomic_t result = 0; __atomic_load(&var->val, &result, __ATOMIC_RELAXED); return result; From b113101c654fc1fd498e76a5a2315851da0ca3ef Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 14 Jun 2023 18:02:08 -0700 Subject: [PATCH 4/6] Make feature test check for locking --- tests/features/S2N_ATOMIC_SUPPORTED.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/features/S2N_ATOMIC_SUPPORTED.c b/tests/features/S2N_ATOMIC_SUPPORTED.c index d371aaee3a4..e9244361f4c 100644 --- a/tests/features/S2N_ATOMIC_SUPPORTED.c +++ b/tests/features/S2N_ATOMIC_SUPPORTED.c @@ -14,10 +14,17 @@ */ #include +#include int main() { + /* Atomic builtins are supported by gcc 4.7.3 and later. */ sig_atomic_t atomic = 0; __atomic_test_and_set(&atomic, __ATOMIC_RELAXED); __atomic_clear(&atomic, __ATOMIC_RELAXED); + + /* _Static_assert is supported for C99 by gcc 4.6 and later, + * so using it here shouldn't limit use of the atomic builtins. */ + _Static_assert(__atomic_always_lock_free(sizeof(sig_atomic_t), NULL), + "Atomic operations in this environment would require locking"); return 0; } From 15445c23228e9bdc28f7f055b7e687e320129bb6 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 14 Jun 2023 18:11:47 -0700 Subject: [PATCH 5/6] Use same methods in feature test as in source --- tests/features/S2N_ATOMIC_SUPPORTED.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/features/S2N_ATOMIC_SUPPORTED.c b/tests/features/S2N_ATOMIC_SUPPORTED.c index e9244361f4c..3e2b29407d6 100644 --- a/tests/features/S2N_ATOMIC_SUPPORTED.c +++ b/tests/features/S2N_ATOMIC_SUPPORTED.c @@ -18,9 +18,9 @@ int main() { /* Atomic builtins are supported by gcc 4.7.3 and later. */ - sig_atomic_t atomic = 0; - __atomic_test_and_set(&atomic, __ATOMIC_RELAXED); - __atomic_clear(&atomic, __ATOMIC_RELAXED); + sig_atomic_t atomic = 0, value = 1; + __atomic_store(&atomic, &value, __ATOMIC_RELAXED); + __atomic_load(&atomic, &value, __ATOMIC_RELAXED); /* _Static_assert is supported for C99 by gcc 4.6 and later, * so using it here shouldn't limit use of the atomic builtins. */ From 04149ee7ce07614c4f9a2e75566a90470c3d41e7 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 14 Jun 2023 18:41:48 -0700 Subject: [PATCH 6/6] Change store/load back to set/clear/test --- tests/unit/s2n_alerts_test.c | 8 +++--- tests/unit/s2n_connection_test.c | 24 ++++++++--------- tests/unit/s2n_early_data_io_api_test.c | 4 +-- tests/unit/s2n_shutdown_test.c | 36 ++++++++++++------------- tls/s2n_alerts.c | 6 ++--- tls/s2n_connection.c | 8 +++--- tls/s2n_connection.h | 8 +++--- tls/s2n_recv.c | 4 +-- tls/s2n_shutdown.c | 8 +++--- utils/s2n_atomic.c | 23 +++++++++++----- utils/s2n_atomic.h | 7 ++--- 11 files changed, 74 insertions(+), 62 deletions(-) diff --git a/tests/unit/s2n_alerts_test.c b/tests/unit/s2n_alerts_test.c index e930c8b551a..9131ee57a1c 100644 --- a/tests/unit/s2n_alerts_test.c +++ b/tests/unit/s2n_alerts_test.c @@ -106,7 +106,7 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT)); /* Verify state prior to alert */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); /* Write and process the alert */ EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->in, not_close_notify_alert, sizeof(not_close_notify_alert))); @@ -115,7 +115,7 @@ int main(int argc, char **argv) EXPECT_FAILURE_WITH_ERRNO(s2n_process_alert_fragment(conn), S2N_ERR_ALERT); /* Verify state after alert */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_SUCCESS(s2n_connection_free(conn)); } @@ -126,14 +126,14 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT)); /* Verify state prior to alert */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); /* Write and process the alert */ EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->in, close_notify_alert, sizeof(close_notify_alert))); EXPECT_SUCCESS(s2n_process_alert_fragment(conn)); /* Verify state after alert */ - EXPECT_TRUE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_TRUE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_SUCCESS(s2n_connection_free(conn)); } diff --git a/tests/unit/s2n_connection_test.c b/tests/unit/s2n_connection_test.c index d982890ad08..e12cc656e20 100644 --- a/tests/unit/s2n_connection_test.c +++ b/tests/unit/s2n_connection_test.c @@ -811,24 +811,24 @@ int main(int argc, char **argv) EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); /* Close write */ - s2n_atomic_store(&conn->write_closed, true); + s2n_atomic_flag_set(&conn->write_closed); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - s2n_atomic_store(&conn->write_closed, false); + s2n_atomic_flag_clear(&conn->write_closed); /* Close read */ - s2n_atomic_store(&conn->read_closed, true); + s2n_atomic_flag_set(&conn->read_closed); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - s2n_atomic_store(&conn->read_closed, false); + s2n_atomic_flag_clear(&conn->read_closed); /* Close both */ - s2n_atomic_store(&conn->read_closed, true); - s2n_atomic_store(&conn->write_closed, true); + s2n_atomic_flag_set(&conn->read_closed); + s2n_atomic_flag_set(&conn->write_closed); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -849,24 +849,24 @@ int main(int argc, char **argv) EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); /* Close write */ - s2n_atomic_store(&conn->write_closed, true); + s2n_atomic_flag_set(&conn->write_closed); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - s2n_atomic_store(&conn->write_closed, false); + s2n_atomic_flag_clear(&conn->write_closed); /* Close read */ - s2n_atomic_store(&conn->read_closed, true); + s2n_atomic_flag_set(&conn->read_closed); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - s2n_atomic_store(&conn->read_closed, false); + s2n_atomic_flag_clear(&conn->read_closed); /* Close both */ - s2n_atomic_store(&conn->read_closed, true); - s2n_atomic_store(&conn->write_closed, true); + s2n_atomic_flag_set(&conn->read_closed); + s2n_atomic_flag_set(&conn->write_closed); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); diff --git a/tests/unit/s2n_early_data_io_api_test.c b/tests/unit/s2n_early_data_io_api_test.c index 8b05a0b83a5..966da872a56 100644 --- a/tests/unit/s2n_early_data_io_api_test.c +++ b/tests/unit/s2n_early_data_io_api_test.c @@ -373,8 +373,8 @@ int main(int argc, char **argv) /* Pretend we didn't test the above error condition. * The S2N_ERR_BAD_MESSAGE error triggered S2N to close the connection. */ - s2n_atomic_store(&server_conn->write_closed, false); - s2n_atomic_store(&client_conn->write_closed, false); + s2n_atomic_flag_clear(&server_conn->write_closed); + s2n_atomic_flag_clear(&client_conn->write_closed); /* Read the remaining early data properly */ EXPECT_SUCCESS(s2n_recv_early_data(server_conn, actual_payload, sizeof(actual_payload), diff --git a/tests/unit/s2n_shutdown_test.c b/tests/unit/s2n_shutdown_test.c index 00d9b37a4cf..e0f1e09c2a3 100644 --- a/tests/unit/s2n_shutdown_test.c +++ b/tests/unit/s2n_shutdown_test.c @@ -66,7 +66,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -78,7 +78,7 @@ int main(int argc, char **argv) /* Verify state after shutdown attempt */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); @@ -109,7 +109,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -123,7 +123,7 @@ int main(int argc, char **argv) /* Verify state after shutdown attempt */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); @@ -146,7 +146,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -173,7 +173,7 @@ int main(int argc, char **argv) /* Verify state after shutdown attempt */ EXPECT_TRUE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); @@ -195,7 +195,7 @@ int main(int argc, char **argv) /* Verify state prior to alert */ EXPECT_FALSE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -204,7 +204,7 @@ int main(int argc, char **argv) /* Verify state after shutdown */ EXPECT_FALSE(s2n_handshake_is_complete(conn)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); /* Fully closed: we don't worry about truncating data */ @@ -225,7 +225,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_connection_set_io_stuffers(&input, &output, conn)); /* Verify state prior to alert */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -234,7 +234,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(blocked, S2N_BLOCKED_ON_READ); /* Verify state after shutdown attempt */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); /* Half-close: only write closed */ @@ -257,7 +257,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_connection_set_io_stuffers(&input, &output, conn)); /* Verify state prior to alert */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX)); @@ -266,7 +266,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_process_alert_fragment(conn)); /* Verify state after alert */ - EXPECT_TRUE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_TRUE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE)); EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_READABLE)); @@ -276,7 +276,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(blocked, S2N_NOT_BLOCKED); /* Verify state after shutdown attempt */ - EXPECT_TRUE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_TRUE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); }; @@ -309,7 +309,7 @@ int main(int argc, char **argv) EXPECT_FAILURE_WITH_ERRNO(s2n_shutdown(conn, &blocked), S2N_ERR_ALERT); /* Verify state after shutdown attempt */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); EXPECT_EQUAL(s2n_stuffer_data_available(&output), alert_record_size); @@ -317,7 +317,7 @@ int main(int argc, char **argv) /* Future calls are no-ops */ for (size_t i = 0; i < 5; i++) { EXPECT_SUCCESS(s2n_shutdown(conn, &blocked)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_TRUE(conn->alert_sent); } }; @@ -410,7 +410,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(blocked, S2N_NOT_BLOCKED); /* Verify state after shutdown attempt */ - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); EXPECT_FALSE(conn->alert_sent); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); }; @@ -586,7 +586,7 @@ int main(int argc, char **argv) /* Full close is no-op */ EXPECT_SUCCESS(s2n_shutdown(conn, &blocked)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); }; /* Test: Half close, peer alert, then full close */ @@ -621,7 +621,7 @@ int main(int argc, char **argv) /* Full close is no-op */ EXPECT_SUCCESS(s2n_shutdown(conn, &blocked)); EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED)); - EXPECT_FALSE(s2n_atomic_load(&conn->close_notify_received)); + EXPECT_FALSE(s2n_atomic_flag_test(&conn->close_notify_received)); }; } diff --git a/tls/s2n_alerts.c b/tls/s2n_alerts.c index 9d9cffccee3..4cc54e6c230 100644 --- a/tls/s2n_alerts.c +++ b/tls/s2n_alerts.c @@ -204,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) { - s2n_atomic_store(&conn->read_closed, true); - s2n_atomic_store(&conn->close_notify_received, true); + s2n_atomic_flag_set(&conn->read_closed); + s2n_atomic_flag_set(&conn->close_notify_received); return 0; } @@ -222,7 +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_store(&conn->error_alert_received, true); + s2n_atomic_flag_set(&conn->error_alert_received); POSIX_BAIL(S2N_ERR_ALERT); } } diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index b2e27d4b314..04316e7ad2d 100644 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -1169,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); - s2n_atomic_store(&conn->read_closed, true); - s2n_atomic_store(&conn->write_closed, true); + s2n_atomic_flag_set(&conn->read_closed); + s2n_atomic_flag_set(&conn->write_closed); return S2N_RESULT_OK; } @@ -1548,8 +1548,8 @@ bool s2n_connection_check_io_status(struct s2n_connection *conn, s2n_io_status s return false; } - bool read_closed = s2n_atomic_load(&conn->read_closed); - bool write_closed = s2n_atomic_load(&conn->write_closed); + bool read_closed = s2n_atomic_flag_test(&conn->read_closed); + bool write_closed = s2n_atomic_flag_test(&conn->write_closed); bool full_duplex = !read_closed && !write_closed; /* diff --git a/tls/s2n_connection.h b/tls/s2n_connection.h index 940ef5abcac..b256bb11740 100644 --- a/tls/s2n_connection.h +++ b/tls/s2n_connection.h @@ -271,8 +271,8 @@ struct s2n_connection { bool alert_sent; /* Receiving error or close_notify alerts changes the behavior of s2n_shutdown_send */ - s2n_atomic_bool error_alert_received; - s2n_atomic_bool close_notify_received; + s2n_atomic_flag error_alert_received; + s2n_atomic_flag close_notify_received; /* Our handshake state machine */ struct s2n_handshake handshake; @@ -313,8 +313,8 @@ struct s2n_connection { /* Either the reader or the writer can trigger both sides of the connection * to close in response to a fatal error. */ - s2n_atomic_bool read_closed; - s2n_atomic_bool write_closed; + s2n_atomic_flag read_closed; + s2n_atomic_flag write_closed; /* TLS extension data */ char server_name[S2N_MAX_SERVER_NAME + 1]; diff --git a/tls/s2n_recv.c b/tls/s2n_recv.c index 6a37c8bcb73..5350ba4c5bd 100644 --- a/tls/s2n_recv.c +++ b/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) { - s2n_atomic_store(&conn->read_closed, true); + s2n_atomic_flag_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(s2n_atomic_load(&conn->close_notify_received), S2N_ERR_CLOSED); + POSIX_ENSURE(s2n_atomic_flag_test(&conn->close_notify_received), S2N_ERR_CLOSED); *blocked = S2N_NOT_BLOCKED; return 0; } diff --git a/tls/s2n_shutdown.c b/tls/s2n_shutdown.c index a54c8b3d9cb..6086322a15e 100644 --- a/tls/s2n_shutdown.c +++ b/tls/s2n_shutdown.c @@ -23,7 +23,7 @@ static bool s2n_shutdown_expect_close_notify(struct s2n_connection *conn) { /* No close_notify expected if we already received an error instead */ - if (s2n_atomic_load(&conn->error_alert_received)) { + if (s2n_atomic_flag_test(&conn->error_alert_received)) { return false; } @@ -67,11 +67,11 @@ int s2n_shutdown_send(struct s2n_connection *conn, s2n_blocked_status *blocked) } /* Flush any outstanding data */ - s2n_atomic_store(&conn->write_closed, true); + s2n_atomic_flag_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_atomic_load(&conn->error_alert_received)) { + if (s2n_atomic_flag_test(&conn->error_alert_received)) { return S2N_SUCCESS; } @@ -120,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 (!s2n_atomic_load(&conn->close_notify_received)) { + while (!s2n_atomic_flag_test(&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) { diff --git a/utils/s2n_atomic.c b/utils/s2n_atomic.c index 8496bd40732..cd2597cd0f0 100644 --- a/utils/s2n_atomic.c +++ b/utils/s2n_atomic.c @@ -19,25 +19,36 @@ #include "utils/s2n_safety.h" +static sig_atomic_t set_val = true; +static sig_atomic_t clear_val = false; + S2N_RESULT s2n_atomic_init() { #if S2N_ATOMIC_SUPPORTED && S2N_THREAD_SANITIZER - RESULT_ENSURE(__atomic_always_lock_free(sizeof(s2n_atomic_bool), NULL), S2N_ERR_ATOMIC); + RESULT_ENSURE(__atomic_always_lock_free(sizeof(s2n_atomic_flag), NULL), S2N_ERR_ATOMIC); #endif return S2N_RESULT_OK; } -void s2n_atomic_store(s2n_atomic_bool *var, bool val) +void s2n_atomic_flag_set(s2n_atomic_flag *var) +{ +#if S2N_ATOMIC_SUPPORTED && S2N_THREAD_SANITIZER + __atomic_store(&var->val, &set_val, __ATOMIC_RELAXED); +#else + var->val = set_val; +#endif +} + +void s2n_atomic_flag_clear(s2n_atomic_flag *var) { #if S2N_ATOMIC_SUPPORTED && S2N_THREAD_SANITIZER - sig_atomic_t input = val; - __atomic_store(&var->val, &input, __ATOMIC_RELAXED); + __atomic_store(&var->val, &clear_val, __ATOMIC_RELAXED); #else - var->val = val; + var->val = clear_val; #endif } -bool s2n_atomic_load(s2n_atomic_bool *var) +bool s2n_atomic_flag_test(s2n_atomic_flag *var) { #if S2N_ATOMIC_SUPPORTED && S2N_THREAD_SANITIZER sig_atomic_t result = 0; diff --git a/utils/s2n_atomic.h b/utils/s2n_atomic.h index 9720e42e972..014dc43dd32 100644 --- a/utils/s2n_atomic.h +++ b/utils/s2n_atomic.h @@ -31,11 +31,12 @@ typedef struct { * atomicity for interrupts. */ sig_atomic_t val; -} s2n_atomic_bool; +} s2n_atomic_flag; /* 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_store(s2n_atomic_bool *var, bool value); -bool s2n_atomic_load(s2n_atomic_bool *var); +void s2n_atomic_flag_set(s2n_atomic_flag *var); +void s2n_atomic_flag_clear(s2n_atomic_flag *var); +bool s2n_atomic_flag_test(s2n_atomic_flag *var);