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

ci: Add UBSAN test to the sanitizer #4740

Merged
merged 7 commits into from
Aug 29, 2024
Merged

Conversation

boquan-fang
Copy link
Contributor

@boquan-fang boquan-fang commented Aug 28, 2024

Resolved issues:

Resolves the second requirement #4684. This PR is also a alternative for PR#4734.

Description of changes:

  • Change the file name of buildspec_asan.yml to buildspec_sanitizer.yml.
  • Add -DUBSAN=ON to buildspec_sanitizer.yml.

Call-outs:

  • If additional sanitizers needs to be added here, the runtime of such change needs to be properly tested.
  • Current runtime for such build is 8 minutes and 47 seconds. There is no build runtime regression.
  • Don't enable auto-merge. Need to immediately change the CI task to the new buildspec_sanitizer.yml file as source.

Testing:

Need to prove the one build commands with both ASAN and UBSAN will still check for memory leak and undefined behavior.

  • Remove some fixes from PR#4722 to intentionally trigger undefined behavior.
  • Test the code on CodeBuild, the results are as followed:
226/268 Test #233: s2n_stuffer_test .................................***Failed    0.01 sec
Running /codebuild/output/src652319342/src/github.com/boquan-fang/s2n-tls/tests/unit/s2n_stuffer_test.c ... /codebuild/output/src652319342/src/github.com/boquan-fang/s2n-tls/stuffer/s2n_stuffer.c:443:40: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /codebuild/output/src652319342/src/github.com/boquan-fang/s2n-tls/stuffer/s2n_stuffer.c:443:40

Link: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/AddressSanitizer/batch/AddressSanitizer%3A9f488b1a-2ce9-463b-90b6-ea022fc90dfc?region=us-west-2
The build detects UBSAN violation.

  • With that error in place, remove DEFER_CLEANUP Macro in s2n_record_size_test.c. That should trigger memory leak.
  • Test it on CodeBuild, the builder detects memory leak in that specific test:
170/268 Test #161: s2n_record_size_test .............................***Failed 0.91 sec Running /codebuild/output/src1161013577/src/github.com/boquan-fang/s2n-tls/tests/unit/s2n_record_size_test.c ... ================================================================= ==5305==ERROR: LeakSanitizer: detected memory leaks Direct leak of 4208 byte(s) in 1 object(s) allocated from: #0 0x563e4544706e in __interceptor_malloc (/codebuild/output/src1161013577/src/github.com/boquan-fang/s2n-tls/build/bin/s2n_record_size_test+0x1c806e) (BuildId: a26d759140b97c61726bac02fa2c0fcfceae8dce) #1 0x563e455b16b9 in s2n_mem_malloc_no_mlock_impl /codebuild/output/src1161013577/src/github.com/boquan-fang/s2n-tls/utils/s2n_mem.c:127:12 #2 0x563e455aed60 in s2n_realloc /codebuild/output/src1161013577/src/github.com/boquan-fang/s2n-tls/utils/s2n_mem.c:218:9 #3 0x563e455ae4e8 in s2n_alloc /codebuild/output/src1161013577/src/github.com/boquan-fang/s2n-tls/utils/s2n_mem.c:181:5 #4 0x563e454d4be7 in s2n_connection_new /codebuild/output/src1161013577/src/github.com/boquan-fang/s2n-tls/tls/s2n_connection.c:72:5 #5 0x563e454815a8 in main /codebuild/output/src1161013577/src/github.com/boquan-fang/s2n-tls/tests/unit/s2n_record_size_test.c:75:39 #6 0x7ff4c9c23d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

Link: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/AddressSanitizer/batch/AddressSanitizer%3Ae38cdb87-26a6-4081-888a-faa0aed81dd1?region=us-west-2

  • It also captures all previous UBSAN violations.
  • Therefore, it shows that ASAN and UBSAN check can work together.
    By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

Can you include links to the failed codebuild jobs in your testing section?

codebuild/spec/buildspec_sanitizer.yml Outdated Show resolved Hide resolved
@lrstewart lrstewart added the do_not_merge PR might needs something before merging, even if approved and passing label Aug 28, 2024
* Retain build changes.
@boquan-fang boquan-fang enabled auto-merge (squash) August 28, 2024 23:24
@boquan-fang boquan-fang merged commit 8675d04 into aws:main Aug 29, 2024
36 checks passed
@boquan-fang boquan-fang removed the do_not_merge PR might needs something before merging, even if approved and passing label Aug 29, 2024
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.

4 participants