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

Fixes dynamic loading bug #4024

Merged
merged 16 commits into from
Jun 2, 2023
Merged

Fixes dynamic loading bug #4024

merged 16 commits into from
Jun 2, 2023

Conversation

maddeleine
Copy link
Contributor

@maddeleine maddeleine commented May 25, 2023

Resolved issues:

resolves #3987

Description of changes:

This fixes the crash when libs2n is dynamically loaded and also includes a test for this scenario. Technically this succeeds in both a codebuild and nix run. So I'll keep that around until we decide to fully commit to nix.

Call-outs:

Because the omnibus code changes don't actually affect the PR, and only prevent codebuild from running in our CI, I will leave those to the next PR. But the change to the omnibus build would be:

    - identifier: s2nDynamicLoad
      buildspec: codebuild/spec/buildspec_ubuntu.yml
      env:
        compute-type: BUILD_GENERAL1_SMALL
        image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild
        privileged-mode: true
        variables:
          TESTS: dynamicload
          GCC_VERSION: '9'
          S2N_LIBCRYPTO: openssl-1.1.1

Testing:

Includes a test

Successful codebuild run: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/s2nOmnibus/build/s2nOmnibus%3A20bf6549-10f1-4c5c-aa1d-8c220f0977f9?region=us-west-2

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label May 25, 2023
@maddeleine maddeleine requested review from lrstewart and dougch May 26, 2023 17:40
@maddeleine maddeleine requested review from camshaft and removed request for lrstewart May 30, 2023 16:47
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

Consider making the omnibus changes separate so we get CI runs from codebuild. Also still thinking about how to get this to pass under nix.

codebuild/bin/test_dynamic_load.sh Outdated Show resolved Hide resolved
@maddeleine maddeleine requested review from toidiu and removed request for camshaft May 31, 2023 21:52
Comment on lines +45 to +47
if ((*s2n_cleanup_dl)()) {
exit(1);
}
Copy link
Contributor

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 break

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.

Copy link
Contributor Author

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.

https://linux.die.net/man/3/dlopen
Since glibc 2.2.3, atexit can be used to register an exit handler that is automatically called when a library is unloaded.

Although it is dependent on the glibc version I guess.

Copy link
Contributor

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

codebuild/bin/s2n_dynamic_load_test.c Outdated Show resolved Hide resolved
codebuild/bin/test_dynamic_load.sh Outdated Show resolved Hide resolved
codebuild/bin/test_dynamic_load.sh Outdated Show resolved Hide resolved
@maddeleine maddeleine requested a review from goatgoose June 1, 2023 23:04
nix/README.md Show resolved Hide resolved
nix/README.md Show resolved Hide resolved
nix/shell.sh Show resolved Hide resolved
codebuild/bin/s2n_dynamic_load_test.c Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on thread termination when s2n has been unloaded
5 participants