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

Properly cleanup all fd from tests #4005

Closed
8 tasks done
toidiu opened this issue May 11, 2023 · 2 comments
Closed
8 tasks done

Properly cleanup all fd from tests #4005

toidiu opened this issue May 11, 2023 · 2 comments

Comments

@toidiu
Copy link
Contributor

toidiu commented May 11, 2023

Problem:

Some of our tests dont do proper cleanup of /dev/urandom. Its possible to detect which tests are not doing this by running valgrind with --track-fds=yes flag.

Solution:

Track which tests are not cleaning up and fix the test

  • run valgrind with --track-fds=yes and collect which tests fail
  • Fix tests
    • Fix AF_UNIXsockets that are left opened.
    • Fix /dev/urandom that wasn't closed properly
      • s2n_override_openssl_random_test We replace the cleanup callbacks when we set the new drbg so dont clean it up when we finally exit.
      • Fix AF_INET socketst that are left opened.
  • Exam test_exec_leak.sh to see if it is functional.
  • Build a test to detect any unwanted open file descriptors
@lrstewart
Copy link
Contributor

We should probably also update / add a new test to track this if we're going to fix it.

@boquan-fang
Copy link
Contributor

boquan-fang commented Nov 12, 2024

All leaking file descriptors were collected and fixed by PRs mentioned above. Therefore, we are closing this issue.

Here is a summary log for this issue:

  • Used Valgrind --track-fds option to track which file descriptors were leaking in tests.

    • External customers first detected the issue that our library was leaking /dev/urandom file descriptor.
    • According to them, kernel could clean up some memory, but open file descriptors would be left behind if we didn’t properly clean them up.
    • Valgrind test detected the following leaks:
      • AF_UNIX socket leaks: this type of leaks happened in 25 tests. They could be tracked in fix: fix opened AF_UNIX sockets that didn't call s2n_io_pair_close #4833, and fix: some open AF_UNIX sockets in forked child processes #4834.
        • Some were caused by not properly calling s2n_io_pair_close , so that io_pairs leaked fds.
        • Some were caused by child process terminated in another scope, so that DEFER_CLEANUP wasn’t properly executed.
      • AF_INET socket leaks: this type of leaks could only be found in s2n_self_talk_ktls_test.c, since such type of socket was only used in that test. The pull request for this fix is fix: fix open AF_INET sockets in s2n_self_talk_ktls_test.c #4852
        • We used two processes to connect the client and server sides of a io_pair. The child process did early termination, so DEFER_CLEANUP for io_pair was not exectuted, as well as many other previously allocated memories were not freed.
      • /dev/urandom leaks: we found there are four tests that leaked the /dev/urandom file descriptor. The PR for this fix is fix: close all /dev/urandom open fds #4835.
        • Some tests reset the original rand_cleanup_cb to other functions which prevented urandom to be closed at exit.
          • It happened in s2n_drbg_test.c and s2n_override_openssl_random_test.c.
          • The solution is to call s2n_rand_cleanup first, before we reset those callbacks.
        • s2n_fork_generation_number_teset.c didn’t explicitly exit from a cloned process, which didn’t trigger the atexit handler to run.
        • Logical error in s2n_random_test.c caused the file descriptors to leak.
  • Added a test script in the CI to capture all leaking fds. Now, every PRs will be ran with such check.

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