Skip to content

Commit

Permalink
Fix TSAN s2n_shutdown failures (#4055)
Browse files Browse the repository at this point in the history
  • Loading branch information
lrstewart authored Jun 15, 2023
1 parent f7d65fc commit c9dd66e
Show file tree
Hide file tree
Showing 17 changed files with 210 additions and 89 deletions.
8 changes: 6 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ if(S2N_UNSAFE_FUZZING_MODE)
endif()

if(TSAN)
target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=thread -DS2N_THREAD_SANITIZER=1)
target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
endif()

Expand Down Expand Up @@ -510,7 +510,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}")

Expand Down
1 change: 1 addition & 0 deletions error/s2n_errno.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
1 change: 1 addition & 0 deletions error/s2n_errno.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
30 changes: 30 additions & 0 deletions tests/features/S2N_ATOMIC_SUPPORTED.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* 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>
#include <stddef.h>

int main() {
/* Atomic builtins are supported by gcc 4.7.3 and later. */
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. */
_Static_assert(__atomic_always_lock_free(sizeof(sig_atomic_t), NULL),
"Atomic operations in this environment would require locking");
return 0;
}
1 change: 1 addition & 0 deletions tests/features/S2N_ATOMIC_SUPPORTED.flags
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-Werror
8 changes: 4 additions & 4 deletions tests/unit/s2n_alerts_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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_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)));
Expand All @@ -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_flag_test(&conn->close_notify_received));

EXPECT_SUCCESS(s2n_connection_free(conn));
}
Expand All @@ -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_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(conn->close_notify_received);
EXPECT_TRUE(s2n_atomic_flag_test(&conn->close_notify_received));

EXPECT_SUCCESS(s2n_connection_free(conn));
}
Expand Down
32 changes: 12 additions & 20 deletions tests/unit/s2n_connection_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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_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));
conn->write_closed = 0;
s2n_atomic_flag_clear(&conn->write_closed);

/* Close read */
conn->read_closed = 1;
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));
conn->read_closed = 0;
s2n_atomic_flag_clear(&conn->read_closed);

/* Close both */
conn->read_closed = 1;
conn->write_closed = 1;
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));
Expand All @@ -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_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));
conn->write_closed = 0;
s2n_atomic_flag_clear(&conn->write_closed);

/* Close read */
conn->read_closed = 1;
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));
conn->read_closed = 0;
s2n_atomic_flag_clear(&conn->read_closed);

/* Close both */
conn->read_closed = 1;
conn->write_closed = 1;
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));
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/s2n_early_data_io_api_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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_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),
Expand Down
36 changes: 18 additions & 18 deletions tests/unit/s2n_shutdown_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX));

Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_TRUE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED));

Expand Down Expand Up @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX));

Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_TRUE(conn->alert_sent);
EXPECT_FALSE(s2n_connection_check_io_status(conn, S2N_IO_WRITABLE));

Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX));

Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED));

Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX));

Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_TRUE(conn->alert_sent);

/* Fully closed: we don't worry about truncating data */
Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX));

Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_TRUE(conn->alert_sent);

/* Half-close: only write closed */
Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_FULL_DUPLEX));

Expand All @@ -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_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));
Expand All @@ -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_flag_test(&conn->close_notify_received));
EXPECT_TRUE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED));
};
Expand Down Expand Up @@ -309,15 +309,15 @@ 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_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);

/* 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_flag_test(&conn->close_notify_received));
EXPECT_TRUE(conn->alert_sent);
}
};
Expand Down Expand Up @@ -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_flag_test(&conn->close_notify_received));
EXPECT_FALSE(conn->alert_sent);
EXPECT_TRUE(s2n_connection_check_io_status(conn, S2N_IO_CLOSED));
};
Expand Down Expand Up @@ -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_flag_test(&conn->close_notify_received));
};

/* Test: Half close, peer alert, then full close */
Expand Down Expand Up @@ -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_flag_test(&conn->close_notify_received));
};
}

Expand Down
6 changes: 4 additions & 2 deletions tls/s2n_alerts.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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_flag_set(&conn->read_closed);
s2n_atomic_flag_set(&conn->close_notify_received);
return 0;
}

Expand All @@ -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_flag_set(&conn->error_alert_received);
POSIX_BAIL(S2N_ERR_ALERT);
}
}
Expand Down
Loading

0 comments on commit c9dd66e

Please sign in to comment.