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

fix: close all /dev/urandom open fds #4835

Merged
merged 10 commits into from
Oct 29, 2024
Merged

Conversation

boquan-fang
Copy link
Contributor

@boquan-fang boquan-fang commented Oct 9, 2024

Resolved issues:

Partially solve #4005

Description of changes:

  • Directly call s2n_rand_cleanup to close /dev/urandom in s2n_drbg_test.c and s2n_override_openssl_random_test.c.
    • s2n_drbg_test.c and s2n_override_openssl_random_test.c calls s2n_rand_set_callbacks function which resets the default cleanup callback functions.
    • The rand_cleanup_callback parameter is supposed to close /dev/urandom, but s2n_cleanup no longer calls that function, since that parameter is overwritten to some other clean up functions, like nist_fake_entropy_init_cleanup.
    • Hence, I call s2n_rand_cleanup to run those fake clean up call back functions, and reset those call back functions back to default.
    • In that way, END_TEST will cleanup the default callbacks set by BEGIN_TEST, so that the default callbacks won't leak fds.
    • The solution is to call s2n_rand_cleanup to cleanup the default random callbacks. Then we call s2n_rand_init to reinitiate the randomness engine again.
  • Some other options I tried for the previous problem
    • Implement another possible fix. Look for that comment for reasons why it wouldn't work.
    • Call s2n_rand_set_callbacks before BEGIN_TEST.
      • s2n_drbg_test.c wants to pass two different generators to s2n_rand_set_callbacks, so fix a generator before BEGIN_TEST will change the test itself.
  • Close /dev/urandom in s2n_fork_generation_number_test.c.
    • Call s2n_rand_clean_up() directly for the reason of this comment.
  • Close dev_urandom in s2n_random_test.c for both test 0 and 1.
    • For test 1, we need to close the original fd for dev_urandom before we make that fd invalid.

Call-outs:

  • See PR#4833 and PR#4834 for fixes of other open fds.
  • I didn't add a test in this PR. The test to detect all opened fds will be in a separate PR.

Testing:

  • Test locally.
    • Add --track-fds=yes to CTest memcheck and direct all Valgrind output to MemoryTester.log files.
    • Search in VS Code for Open File Descriptors of /dev/urandom.
      • Previously opened /dev/urandom in tests that are fixed.
    • This is similar to the check in ./codebuild/bin/test_exec_leak.sh.
      # run valgrind with track-fds enabled
      valgrind_log_dir=valgrind_log_dir
      for test_file in detect_exec_leak detect_exec_leak_finish; do
      LD_LIBRARY_PATH="build/lib:$TARGET_LIBCRYPTO_PATH/lib:$LD_LIBRARY_PATH" S2N_VALGRIND=1 \
      valgrind --leak-check=full --show-leak-kinds=all --errors-for-leak-kinds=all \
      --run-libc-freeres=yes -q --gen-suppressions=all --track-fds=yes \
      --leak-resolution=high --undef-value-errors=no --trace-children=yes \
      --suppressions=tests/unit/valgrind.suppressions --log-file="build/$valgrind_log_dir/$test_file" \
      build/bin/$test_file
      # search for all leaked file descriptors, excluding the valgrind_log_dir file
      cat build/$valgrind_log_dir/$test_file | \
      grep "Open file descriptor" | \
      grep --invert-match $valgrind_log_dir \
      && fail "file leak detected while running $test_file"
      done
  • Open fds test will be implemented once PR#4851 is merged.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@boquan-fang boquan-fang marked this pull request as ready for review October 9, 2024 22:47
tests/unit/s2n_drbg_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_fork_generation_number_test.c Outdated Show resolved Hide resolved
@boquan-fang boquan-fang requested a review from goatgoose October 14, 2024 22:42
@boquan-fang boquan-fang enabled auto-merge (squash) October 21, 2024 16:26
@lrstewart lrstewart disabled auto-merge October 21, 2024 17:38
@boquan-fang boquan-fang added the do_not_merge PR might needs something before merging, even if approved and passing label Oct 21, 2024
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.

As discussed offline, this change needs better documentation / explanation. That means some combination of code comments, expanding the PR description, and moving the s2n_rand_cleanup calls closer to the code that requires the cleanup.

It's not obvious that you're calling s2n_rand_cleanup to cleanup/reset custom callbacks, so that END_TEST will cleanup the default callbacks set by BEGIN_TEST, so that the default callbacks won't leak fds.

tests/unit/s2n_random_test.c Outdated Show resolved Hide resolved
@boquan-fang boquan-fang force-pushed the open-fds-urandom branch 2 times, most recently from 1b50164 to b0b49b5 Compare October 21, 2024 22:15
* make comments more precise
* move s2n_rand_cleanup to proper location
tests/unit/s2n_drbg_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_fork_generation_number_test.c Outdated Show resolved Hide resolved
* Change PR comments for them to be more precise
* Move s2n_rand_cleanup to above s2n_rand_set_callbacks
tests/unit/s2n_drbg_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_drbg_test.c Outdated Show resolved Hide resolved
* move s2n_rand_cleanup in s2n_override_openssl_random_test.c back to
  the end to fix the failed unit test
* make comments in s2n_drbg_test.c more precise
* Find out dev_urandom in s2n_override_openssl_random_test.c and close
  it before callbacks are reset
@boquan-fang boquan-fang requested a review from lrstewart October 24, 2024 23:56
* Modify comments
* use s2n_rand_init to restart engine
@boquan-fang boquan-fang removed the do_not_merge PR might needs something before merging, even if approved and passing label Oct 28, 2024
@lrstewart lrstewart requested a review from goatgoose October 28, 2024 23:41
tests/unit/s2n_fork_generation_number_test.c Outdated Show resolved Hide resolved
@boquan-fang boquan-fang enabled auto-merge (squash) October 29, 2024 21:12
@boquan-fang boquan-fang merged commit 9f4baec into aws:main Oct 29, 2024
37 checks passed
@boquan-fang boquan-fang deleted the open-fds-urandom branch December 18, 2024 23:35
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