Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: open file decriptors #4831

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ option(S2N_LTO, "Enables link time optimizations when building s2n-tls." OFF)
option(S2N_STACKTRACE "Enables stacktrace functionality in s2n-tls. Note that this functionality is
only available on platforms that support execinfo." ON)
option(COVERAGE "Enable profiling collection for code coverage calculation" OFF)
option(BUILD_TESTING "Build tests for s2n-tls. By default only unit tests are built." ON)
option(S2N_INTEG_TESTS "Enable the integrationv2 tests" OFF)
option(S2N_FAST_INTEG_TESTS "Enable the integrationv2 with more parallelism, only has effect if S2N_INTEG_TESTS=ON" ON)
option(S2N_INSTALL_S2NC_S2ND "Install the binaries s2nc and s2nd" OFF)
Expand All @@ -41,9 +42,6 @@ option(TSAN "Enable ThreadSanitizer to test thread safety" OFF)
option(ASAN "Enable AddressSanitizer to test memory safety" OFF)
option(SECCOMP "Link with seccomp and run seccomp tests" OFF)

# Turn BUILD_TESTING=ON by default
include(CTest)

file(GLOB API_HEADERS "api/*.h")
file(GLOB API_UNSTABLE_HEADERS "api/unstable/*.h")

Expand Down Expand Up @@ -497,6 +495,29 @@ if (BUILD_TESTING)
########################## configure unit tests ############################
############################################################################

# CTest configuration variables need to be set before include(CTest) is called
set(VALGRIND_DEFAULT " \
--leak-check=full \
--leak-resolution=high \
--trace-children=yes \
-q --error-exitcode=123 \
--error-limit=no \
--num-callers=40 \
--undef-value-errors=no \
--log-fd=2 \
--suppressions=valgrind.suppressions")

# "pedantic valgrind" will error on memory that is "Still Reachable".
# We only run this on OpenSSL 1.1.1 because there are hundreds of false positives in other libcryptos.
# Tracking issue: https://github.com/aws/s2n-tls/issues/4777
if ($ENV{S2N_LIBCRYPTO} MATCHES "openssl-1.1.1")
set(MEMORYCHECK_COMMAND_OPTIONS "${VALGRIND_DEFAULT} --run-libc-freeres=yes --errors-for-leak-kinds=all --show-leak-kinds=all")
else()
set(MEMORYCHECK_COMMAND_OPTIONS "${VALGRIND_DEFAULT} --run-libc-freeres=no")
endif()

set(MEMORYCHECK_TYPE "Valgrind")

set(UNIT_TEST_ENVS S2N_DONT_MLOCK=1)
if (TSAN OR ASAN)
set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} S2N_ADDRESS_SANITIZER=1)
Expand Down Expand Up @@ -525,6 +546,8 @@ if (BUILD_TESTING)
endif()
message(STATUS "Running tests with environment: ${UNIT_TEST_ENVS}")

include(CTest)

############################################################################
############################ build unit tests ##############################
############################################################################
Expand Down
72 changes: 72 additions & 0 deletions codebuild/spec/buildspec_valgrind.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
---
# 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.
version: 0.2

batch:
build-list:
- identifier: gcc_awslc
env:
compute-type: BUILD_GENERAL1_LARGE
image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu22codebuild
variables:
S2N_LIBCRYPTO: awslc
COMPILER: gcc
- identifier: gcc_openssl_3_0
env:
compute-type: BUILD_GENERAL1_LARGE
image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu22codebuild
variables:
S2N_LIBCRYPTO: openssl-3.0
COMPILER: gcc
- identifier: gcc_openssl_1_1_1
env:
compute-type: BUILD_GENERAL1_LARGE
image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild
variables:
S2N_LIBCRYPTO: openssl-1.1.1
COMPILER: gcc
- identifier: gcc_openssl_1_0_2
env:
compute-type: BUILD_GENERAL1_LARGE
image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu22codebuild
variables:
S2N_LIBCRYPTO: openssl-1.0.2
COMPILER: gcc

phases:
pre_build:
commands:
- |
if [ -d "third-party-src" ]; then
cd third-party-src;
fi
- /usr/bin/$COMPILER --version
build:
on-failure: ABORT
commands:
- |
cmake . -Bbuild \
-DCMAKE_C_COMPILER=/usr/bin/$COMPILER \
-DCMAKE_PREFIX_PATH=/usr/local/$S2N_LIBCRYPTO \
-DCMAKE_BUILD_TYPE=RelWithDebInfo
- cmake --build ./build -- -j $(nproc)
post_build:
on-failure: ABORT
commands:
- |
S2N_VALGRIND=1 \
CTEST_PARALLEL_LEVEL=$(nproc) \
CTEST_OUTPUT_ON_FAILURE=1 \
cmake --build build/ --target test \
-- ARGS="--test-action memcheck"
8 changes: 6 additions & 2 deletions tests/unit/s2n_alerts_protocol_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_connection_set_blinding(client, S2N_SELF_SERVICE_BLINDING));
EXPECT_SUCCESS(s2n_connection_set_config(client, config));

DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
struct s2n_test_io_pair io_pair = { 0 };
Copy link
Contributor

@goatgoose goatgoose Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't s2n_io_pair_close() close both the client and server file descriptors? Is it because s2n_io_pair_close() will abort if the client is already closed?

EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
EXPECT_SUCCESS(s2n_connections_set_io_pair(client, server, &io_pair));

Expand All @@ -573,6 +573,8 @@ int main(int argc, char **argv)
S2N_ERR_CLOSED);
EXPECT_FALSE(s2n_connection_check_io_status(receiver, S2N_IO_READABLE));
}
/* Close the other end of pipe */
EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, 1 - mode));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - S2N_PEER_MODE makes this slightly clearer:

Suggested change
EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, 1 - mode));
EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, S2N_PEER_MODE(mode)));

};

/* Test: With partial read */
Expand All @@ -594,7 +596,7 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_connection_set_blinding(client, S2N_SELF_SERVICE_BLINDING));
EXPECT_SUCCESS(s2n_connection_set_config(client, &partial_write_config_copy));

DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
struct s2n_test_io_pair io_pair = { 0 };
EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
EXPECT_SUCCESS(s2n_connections_set_io_pair(client, server, &io_pair));

Expand Down Expand Up @@ -629,6 +631,8 @@ int main(int argc, char **argv)
EXPECT_FAILURE_WITH_ERRNO(s2n_recv(receiver, data, sizeof(data), &blocked),
S2N_ERR_CLOSED);
}
/* Close the other end of pipe */
EXPECT_SUCCESS(s2n_io_pair_close_one_end(&io_pair, 1 - mode));
};
};

Expand Down
4 changes: 2 additions & 2 deletions tests/unit/s2n_certificate_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(tls12_client_conn, "test_all_tls12"));
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(tls12_server_conn, "test_all_tls12"));

struct s2n_test_io_pair io_pair = { 0 };
DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
EXPECT_SUCCESS(s2n_connections_set_io_pair(tls12_client_conn, tls12_server_conn, &io_pair));
EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(tls12_server_conn, tls12_client_conn));
Expand All @@ -719,7 +719,7 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(tls13_client_conn, "default_tls13"));
EXPECT_SUCCESS(s2n_connection_set_cipher_preferences(tls13_server_conn, "default_tls13"));

struct s2n_test_io_pair io_pair = { 0 };
DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
EXPECT_SUCCESS(s2n_connections_set_io_pair(tls13_client_conn, tls13_server_conn, &io_pair));
EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(tls13_server_conn, tls13_client_conn));
Expand Down
Loading
Loading