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

test(bindings): run unit tests under asan #4948

Merged
merged 17 commits into from
Dec 4, 2024
Merged

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Dec 4, 2024

Description of changes:

We want to ensure that none of our unit tests are leaking memory.

We currently use "checkers" for some of our tests, but this requires us to manually annotate each test with the checkers::test macro.

Instead of doing that, we can just use Rust's existing address sanitizer support to check for memory leaks in all of the tests.

Testing:

All existing tests should pass.

Additionally, I pushed a commit containing a memory leak here. You can see the failed asan job for that commit here

 =================================================================
==8298==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x55c17b9a2eaf in malloc /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:68:3
    #1 0x55c17ba2ce2b in std::sys::alloc::unix::_$LT$impl$u20$core..alloc..global..GlobalAlloc$u20$for$u20$std..alloc..System$GT$::alloc::h3cfff84b03a4b754 /home/runner/.rustup/toolchains/nightly-2024-12-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/alloc/unix.rs:14:22
    #2 0x55c17ba22589 in _$LT$checkers..allocator..Allocator$LT$T$GT$$u20$as$u20$core..alloc..global..GlobalAlloc$GT$::alloc::h457eba1cc841aa88 /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/checkers-0.6.3/src/allocator.rs:54:19
    #3 0x55c17ba67a91 in __rust_alloc /home/runner/work/s2n-tls/s2n-tls/bindings/rust/s2n-tls/src/lib.rs:11:19
    #4 0x55c17ba78fd4 in alloc::alloc::Global::alloc_impl::h3f5294fc6a51959a /home/runner/.rustup/toolchains/nightly-2024-12-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:195:73
    #5 0x55c17ba79388 in _$LT$alloc..alloc..Global$u20$as$u20$core..alloc..Allocator$GT$::allocate::h5d31da0bd5bc3bca /home/runner/.rustup/toolchains/nightly-2024-12-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:257:9
    #6 0x55c17ba689b6 in s2n_tls::testing::s2n_tls::tests::handshake_default_tls13::_$u7b$$u7b$closure$u7d$$u7d$::h335262bad34403ec /home/runner/work/s2n-tls/s2n-tls/bindings/rust/s2n-tls/src/testing/s2n_tls.rs:25:33
    #7 0x55c17bb4d6aa in test::types::RunnableTest::run::h5819b201521a79fb /home/runner/.rustup/toolchains/nightly-2024-12-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/types.rs:145:40
    <SNIP>

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 Dec 4, 2024
Otherwise it interferes with the generate command, i think
I think most of our overrides are broken, because they seem to be
defined per-directory without any sort of hierarchy
I am tired of fighting with overrides.
I hate debugging remote things...
I am getting undefined symbol error, but most of the other stuff seems
to be running successfully? Honestly pretty confused on this one.

If it was really libasan errors, I would expect that _all_ of the tests
would fail?
Actually, the issue was that I wasn't linking asan for the doc tests.
Add a fake leak to make it show up in the CI output
This reverts commit 4128517.
@jmayclin jmayclin marked this pull request as ready for review December 4, 2024 02:55
.github/workflows/ci_rust.yml Outdated Show resolved Hide resolved
# Rust is generally memory safe, but our bindings contain a large amount of unsafe
# code. Additionally, "safe" code doesn't guarentee that the code is free of
# memory leaks.
asan-unit-tests:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great idea. We should do the same in s2n-quic 🙂

@jmayclin jmayclin enabled auto-merge (squash) December 4, 2024 23:07
@jmayclin jmayclin merged commit 77628b7 into aws:main Dec 4, 2024
39 checks passed
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