aws / s2n-tls

An implementation of the TLS/SSL protocols
https://aws.github.io/s2n-tls/usage-guide/
Apache License 2.0
4.53k stars 707 forks source link

Tracking issue: enable pedantic Valgrind testing #3758

Open toidiu opened 1 year ago

toidiu commented 1 year ago

Problem:

As https://github.com/aws/s2n-tls/pull/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 https://github.com/aws/s2n-tls/pull/3506).

Therefore the goal of the MVP is to

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

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
}

jmayclin commented 9 months ago

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