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

Tracking issue: enable pedantic Valgrind testing #3758

Open
4 of 8 tasks
toidiu opened this issue Jan 14, 2023 · 1 comment
Open
4 of 8 tasks

Tracking issue: enable pedantic Valgrind testing #3758

toidiu opened this issue Jan 14, 2023 · 1 comment

Comments

@toidiu
Copy link
Contributor

toidiu commented Jan 14, 2023

Problem:

As #3506 demonstrated we were not doing proper cleanup. This was not caught by Vangrind because by default Valgrind ignores "Still reachable" leaks (For more info see https://valgrind.org/docs/manual/mc-manual.html). This might be a safe assumption for applications but not for our library. We should therefore enable --errors-for-leak-kinds=all check; aka run Valgrind in pedantic mode.

We currently run Valgrind for multiple compilers and libcryptos. Unfortunately, each does allocate/cleanup in a different manner and can require special code. However, I argue that pedantic mode can be enabled incrementally and that we get 80% of the benefit from enabling pedantic for a single combination (we would have caught the leak in #3506).

Therefore the goal of the MVP is to

  • enable pedantic mode in CI
  • enable Valgrind pedantic mode for OSSL 1.1.1 and GCC_9
  • add suppression if needed to unblock the task

Post MVP we should incrementally enable pedantic mode for all combinations and all tests (remove suppression).

MVP

Post MVP:

Memory leaks on remaining Libcrypto tests

  • Memory failures for awslc-fips is involved and needs to be debugged. For OpenSSL-1.0.2, the leaks seem to be coming from a single test.

  • openssl-1.0.2andopenssl-1.0.2-fips: below are the memory leaks for the openssl-1.0.2 librcrypto.

`openssl-1.0.2` and `openssl-1.0.2-fips failures`
Running s2n_init_test.c                                    ... Running s2n_init_test.c                                    ... PASSED         42 tests
==6017== 24 bytes in 1 blocks are still reachable in loss record 6 of 16
==6017==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6017==    by 0x4EA3C49: default_malloc_ex (mem.c:79)
==6017==    by 0x4EA4306: CRYPTO_malloc (mem.c:346)
==6017==    by 0x4F8F040: lh_insert (lhash.c:210)
==6017==    by 0x4F927C2: int_thread_set_item (err.c:520)
==6017==    by 0x4F93F43: ERR_get_state (err.c:1063)
==6017==    by 0x4F92D35: ERR_put_error (err.c:728)
==6017==    by 0x4F84272: BIO_new_file (bss_file.c:175)
==6017==    by 0x4FE6A5C: X509_load_cert_crl_file (by_file.c:252)
==6017==    by 0x4FE6544: by_file_ctrl (by_file.c:107)
==6017==    by 0x4FE3345: X509_LOOKUP_ctrl (x509_lu.c:120)
==6017==    by 0x4FD9704: X509_STORE_set_default_paths (x509_d2.c:72)
==6017==    by 0x132BD7: s2n_x509_trust_store_from_system_defaults (s2n_x509_validator.c:72)
==6017==    by 0x122D8D: s2n_config_init (s2n_config.c:116)
==6017==    by 0x12350B: s2n_config_defaults_init (s2n_config.c:237)
==6017==    by 0x120B1A: s2n_init (s2n_init.c:74)
==6017==    by 0x120159: s2n_init_success_cb (s2n_init_test.c:32)
==6017==    by 0x52DE6DA: start_thread (pthread_create.c:463)
==6017==    by 0x561761E: clone (clone.S:95)
==6017== 
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: reachable
   fun:malloc
   fun:default_malloc_ex
   fun:CRYPTO_malloc
   fun:lh_insert
   fun:int_thread_set_item
   fun:ERR_get_state
   fun:ERR_put_error
   fun:BIO_new_file
   fun:X509_load_cert_crl_file
   fun:by_file_ctrl
   fun:X509_LOOKUP_ctrl
   fun:X509_STORE_set_default_paths
   fun:s2n_x509_trust_store_from_system_defaults
   fun:s2n_config_init
   fun:s2n_config_defaults_init
   fun:s2n_init
   fun:s2n_init_success_cb
   fun:start_thread
   fun:clone
}
==6017== 600 bytes in 1 blocks are still reachable in loss record 16 of 16
==6017==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6017==    by 0x4EA3C49: default_malloc_ex (mem.c:79)
==6017==    by 0x4EA4306: CRYPTO_malloc (mem.c:346)
==6017==    by 0x4F93E83: ERR_get_state (err.c:1053)
==6017==    by 0x4F92D35: ERR_put_error (err.c:728)
==6017==    by 0x4F84272: BIO_new_file (bss_file.c:175)
==6017==    by 0x4FE6A5C: X509_load_cert_crl_file (by_file.c:252)
==6017==    by 0x4FE6544: by_file_ctrl (by_file.c:107)
==6017==    by 0x4FE3345: X509_LOOKUP_ctrl (x509_lu.c:120)
==6017==    by 0x4FD9704: X509_STORE_set_default_paths (x509_d2.c:72)
==6017==    by 0x132BD7: s2n_x509_trust_store_from_system_defaults (s2n_x509_validator.c:72)
==6017==    by 0x122D8D: s2n_config_init (s2n_config.c:116)
==6017==    by 0x12350B: s2n_config_defaults_init (s2n_config.c:237)
==6017==    by 0x120B1A: s2n_init (s2n_init.c:74)
==6017==    by 0x120159: s2n_init_success_cb (s2n_init_test.c:32)
==6017==    by 0x52DE6DA: start_thread (pthread_create.c:463)
==6017==    by 0x561761E: clone (clone.S:95)
==6017== 
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: reachable
   fun:malloc
   fun:default_malloc_ex
   fun:CRYPTO_malloc
   fun:ERR_get_state
   fun:ERR_put_error
   fun:BIO_new_file
   fun:X509_load_cert_crl_file
   fun:by_file_ctrl
   fun:X509_LOOKUP_ctrl
   fun:X509_STORE_set_default_paths
   fun:s2n_x509_trust_store_from_system_defaults
   fun:s2n_config_init
   fun:s2n_config_defaults_init
   fun:s2n_init
   fun:s2n_init_success_cb
   fun:start_thread
   fun:clone
}
@toidiu toidiu changed the title Tracking issue: wnable pedantic Valgrind testing Tracking issue: enable pedantic Valgrind testing Jan 14, 2023
@toidiu toidiu added the s2n-core team label Jan 17, 2023
This was referenced Feb 14, 2023
@jmayclin
Copy link
Contributor

AWS-LC is showing a number of leaks in different functions. They mostly seem to be in certificate related functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@toidiu @jmayclin and others