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: some open AF_UNIX sockets in forked child processes #4834

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

boquan-fang
Copy link
Contributor

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

Resolved issues:

Partially solve #4005

Description of changes:

  • Use s2n_io_pair_close and s2n_io_pair_close_one_end to close AF_UNIX sockets in tests that have fork().
    • Child processes are often forgot to be cleaned up.

Call-outs:

  • See PR#4833 and PR#4835 for fixing 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 AF_UNIX.
      • Previously opened AF_UNIX sockets in tests that are fixed are gone.
    • 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

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
* move `s2n_io_pair_close_one_end` before `s2n_cleanup`
* move s2n_io_pair_close_one_end to be right above s2n_cleanup so that
  it doesn't cause the test to fail
@boquan-fang boquan-fang merged commit eec5713 into aws:main Oct 14, 2024
38 checks passed
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