-
Notifications
You must be signed in to change notification settings - Fork 712
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
Fixes dynamic loading bug #4024
Merged
Merged
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
2f5f730
Adds test for crash
maddeleine 1899e9f
Added script to compile dynamic load
maddeleine 0eea770
Added codebuild logic
maddeleine 14e24dc
Fixed crash
maddeleine ce61ba5
Fixed linking issue
maddeleine 3c5694f
Added script
maddeleine d66ec4e
clang-format
maddeleine 8035a51
Moved test file
maddeleine af836f5
Forgot file extension
maddeleine 34f0d2e
Fixed file location
maddeleine 8cf3212
Removing omnibus changes until the next PR
maddeleine 288280b
Adds dynamic load test to nix
maddeleine 0d972fb
PR feedback
maddeleine 10c5884
format change
maddeleine 578aa88
Fixed comment
maddeleine 93f9811
Merge branch 'main' into dl_fix
maddeleine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/* | ||
* 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 <dlfcn.h> | ||
#include <pthread.h> | ||
#include <stdio.h> | ||
#include <stdlib.h> | ||
|
||
static void *s2n_load_dynamic_lib(void *ctx) | ||
{ | ||
const char *s2n_so_path = ctx; | ||
|
||
void *s2n_so = dlopen(s2n_so_path, RTLD_NOW); | ||
if (!s2n_so) { | ||
exit(1); | ||
} | ||
|
||
int (*s2n_init_dl)(void) = NULL; | ||
*(void **) (&s2n_init_dl) = dlsym(s2n_so, "s2n_init"); | ||
if (dlerror()) { | ||
exit(1); | ||
} | ||
|
||
int (*s2n_cleanup_dl)(void) = NULL; | ||
*(void **) (&s2n_cleanup_dl) = dlsym(s2n_so, "s2n_cleanup"); | ||
if (dlerror()) { | ||
exit(1); | ||
} | ||
|
||
if ((*s2n_init_dl)()) { | ||
exit(1); | ||
} | ||
if ((*s2n_cleanup_dl)()) { | ||
exit(1); | ||
} | ||
|
||
if (dlclose(s2n_so)) { | ||
exit(1); | ||
} | ||
|
||
return NULL; | ||
} | ||
|
||
int main(int argc, char *argv[]) | ||
{ | ||
if (argc != 2) { | ||
printf("Usage: s2n_dynamic_load_test <path to s2n shared object>\n"); | ||
exit(1); | ||
} | ||
|
||
/* s2n-tls library can be dynamically loaded and cleaned up safely | ||
* | ||
* We can't use any s2n test macros because then the compiler gets | ||
* confused about whether or not to link the s2n functions. | ||
maddeleine marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
{ | ||
pthread_t thread_id = { 0 }; | ||
if (pthread_create(&thread_id, NULL, &s2n_load_dynamic_lib, argv[1])) { | ||
exit(1); | ||
} | ||
if (pthread_join(thread_id, NULL)) { | ||
exit(1); | ||
} | ||
} | ||
|
||
return 0; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
#!/usr/bin/env bash | ||
# 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. | ||
# | ||
|
||
# This script compiles s2n-tls as a shared library and compiles a test | ||
# without linking to the library. This enables us to test behavior when | ||
# s2n-tls is dynamically loaded. | ||
|
||
WORK_DIR=$1 | ||
|
||
if [ ! -z "$NIX_STORE" ]; then | ||
OPENSSL=$(which openssl) | ||
LIBCRYPTO_ROOT=$(nix-store --query $OPENSSL) | ||
else | ||
source codebuild/bin/s2n_setup_env.sh | ||
fi | ||
|
||
S2N_BUILD_ARGS=(-H. -DCMAKE_PREFIX_PATH=$LIBCRYPTO_ROOT -DBUILD_TESTING=OFF) | ||
|
||
# create installation dir with libs2n.so | ||
if [ ! -d $WORK_DIR/s2n-install-shared ]; then | ||
(set -x; cmake -B$WORK_DIR/s2n-build-shared -DCMAKE_INSTALL_PREFIX=$WORK_DIR/s2n-install-shared -DBUILD_SHARED_LIBS=ON ${S2N_BUILD_ARGS[@]}) | ||
(set -x; cmake --build $WORK_DIR/s2n-build-shared --target install -- -j $(nproc)) | ||
fi | ||
|
||
# Compile the test file | ||
$CC -Wl,-rpath $LIBCRYPTO_ROOT -o s2n_dynamic_load_test codebuild/bin/s2n_dynamic_load_test.c -ldl -lpthread | ||
|
||
LDD_OUTPUT=$(ldd s2n_dynamic_load_test) | ||
|
||
# Confirm executable doesn't have libs2n.so loaded | ||
if echo "$LDD_OUTPUT" | grep -q libs2n; then | ||
echo "test failure: libs2n should not appear in ldd output" | ||
exit 1 | ||
fi | ||
|
||
# Run the test with the path to libs2n | ||
echo "Running s2n_dynamic_load_test" | ||
LD_LIBRARY_PATH=$LIBCRYPTO_ROOT/lib ./s2n_dynamic_load_test $WORK_DIR/s2n-install-shared/lib/libs2n.so | ||
returncode=$? | ||
if [ $returncode -ne 0 ]; then | ||
echo "test failure: s2n_dynamic_load_test did not succeed" | ||
exit 1 | ||
fi | ||
echo "Passed s2n_dynamic_load_test" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a test case scenario where we dont call
cleanup
and verify that this doesnt breakThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't call cleanup this will blow up because the dangling destructor will get invoked after unload. If you want a solution where that doesn't happen then you'd need to completely rethink how thread locals and cleanup are managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dlclose call will technically calls s2n_cleanup because of our atexit handler. So there wouldn't be a dangling destructor in this case.
Although it is dependent on the glibc version I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I expect this to not fail if an application fails to all s2n_cleanup since the atexit callback will be invoked. Do we want to test that case here as well to confirm that