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.

Sign up for GitHub

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

Jump to bottom

fix(test): narrow valgrind suppressions #4369

Merged
merged 11 commits into from
Mar 20, 2024
Merged

fix(test): narrow valgrind suppressions #4369

merged 11 commits into from
Mar 20, 2024

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Jan 18, 2024 •
edited
Loading

Resolved issues:

Resolves #4363
Resolves #1600

Description of changes:

Suppression Update: Prefer Narrow

  • remove broad fun:main wildcard leak.
  • add in narrower suppressions to address to specific leaks we are aware of

remove aws-lc from pedantic valgrind

  • It would require thousands of lines of suppressions to run this in CI: look upon them
  • Rather than turning it on and ignoring everything, we should just turn it off. That is more honest.

Fix some memory errors

  • fix memory leaks in s2n_examples_test.c found in refactor: ossl x509 parsing #4351
  • fix memory leaks in s2n_key_update_threads_test.c
  • fix s2n_self_talk_alpn_test.c. This test had many copy pasted cases and was difficult to read, so I refactored it into individual test cases.

Valgrind Command Restructuring

  • valgrind flags on new lines: make changes more visible and git blame more informative
  • "odder" error code 123: previous error code 9 was boring and I assumed it was os-caused
  • add --error-limit=no to show all errors in CI

Testing:

  1. remove broad valgrind suppression
  2. run valgrind tests to see errors
  3. fix some errors
  4. add narrow suppressions for remaining errors

All current CI should pass on this PR.

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 18, 2024
@jmayclin jmayclin force-pushed the fix-valgrind-errors branch 11 times, most recently from 691b0fb to 89de95d Compare January 18, 2024 23:56
@jmayclin jmayclin mentioned this pull request Jan 19, 2024
Merged
@jmayclin jmayclin force-pushed the fix-valgrind-errors branch 4 times, most recently from 048b109 to 12329bb Compare January 19, 2024 02:27
@jmayclin jmayclin marked this pull request as ready for review January 19, 2024 03:24
@jmayclin jmayclin requested a review from dougch as a code owner January 19, 2024 03:24
toidiu
toidiu reviewed Jan 26, 2024
View reviewed changes
tests/unit/Makefile Show resolved Hide resolved
codebuild/bin/s2n_codebuild.sh Show resolved Hide resolved
@toidiu
Copy link
Contributor

toidiu commented Jan 26, 2024

Attaching the Tracking issue #3758

@jmayclin jmayclin requested a review from toidiu January 27, 2024 03:04
goatgoose
goatgoose reviewed Jan 30, 2024
View reviewed changes
tests/unit/s2n_self_talk_alpn_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_self_talk_alpn_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_self_talk_alpn_test.c Show resolved Hide resolved
@jmayclin
address pr feedback
b3dd054 
- correct client protocol length
- add not null assertions
- use predefined bytearray equal macro
goatgoose
goatgoose approved these changes Jan 31, 2024
View reviewed changes
tests/unit/s2n_self_talk_alpn_test.c Outdated Show resolved Hide resolved
@jmayclin jmayclin mentioned this pull request Jan 31, 2024
Closed
@jmayclin
address pr feedback
2910e92 
- clang-format does a minor self destruct if you don't include trailing
  commas, so include trailing commas.
@jmayclin
update comment
6b83d98 
- remove the fun from the comment
@jmayclin jmayclin requested review from lrstewart and removed request for toidiu March 6, 2024 20:34
dougch and others added 2 commits March 13, 2024 15:54
@dougch
Merge branch 'main' into fix-valgrind-errors
43fb856
@jmayclin
address pr feedback
8a0bc57 
- remove gen suppressions on the pedantic valgrind path
@dougch dougch enabled auto-merge (squash) March 14, 2024 22:24
@dougch dougch disabled auto-merge March 14, 2024 23:04
@jmayclin jmayclin enabled auto-merge (squash) March 19, 2024 22:12
@jmayclin jmayclin mentioned this pull request Mar 20, 2024
Merged
@jmayclin jmayclin merged commit cc4a967 into aws:main Mar 20, 2024
31 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Apr 2, 2024
Merged
@jmayclin jmayclin deleted the fix-valgrind-errors branch July 1, 2024 07:25
@boquan-fang boquan-fang mentioned this pull request Sep 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toidiu toidiu toidiu left review comments

@goatgoose goatgoose goatgoose approved these changes

@lrstewart lrstewart lrstewart approved these changes

@dougch dougch dougch approved these changes

Assignees
No one assigned
Labels
s2n-core team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

odd cleanup behavior under valgrind Fix known memory leaks in self-talk tests
5 participants
@jmayclin @toidiu @goatgoose @lrstewart @dougch