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(bindings): prevent temp connection free after panic #5067

Merged
merged 5 commits into from
Jan 31, 2025

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Jan 28, 2025

Description of changes:

We should never be freeing the temporary connection in with_context, even during an unwind. To prevent this make the connection ManuallyDrop during initialization.

Secondly, with_context may be invoked concurrently, so we should use context not context_mut.
Moved to #5701

Testing:

I added a unit test which confirms that the connection is not freed during a panic. I confirmed that the old behavior causes this test to fail.

~/workspace/s2n-tls/bindings/rust/extended$ cargo test panic_does_not_free_connection -- --nocapture

running 1 test
thread 'callbacks::tests::panic_does_not_free_connection' panicked at 'a panic in customer code', s2n-tls/src/callbacks.rs:121:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'callbacks::tests::panic_does_not_free_connection' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', s2n-tls/src/callbacks.rs:141:9
error: test failed, to rerun pass '-p s2n-tls --lib'

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 Jan 28, 2025
@jmayclin jmayclin requested a review from lrstewart January 30, 2025 00:26
@jmayclin jmayclin added this pull request to the merge queue Jan 31, 2025
Merged via the queue into aws:main with commit d80e093 Jan 31, 2025
44 checks passed
@jmayclin jmayclin deleted the funky-free-fix branch January 31, 2025 18:55
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.

3 participants