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: fix opened AF_UNIX sockets that didn't call s2n_io_pair_close #4833

Merged
merged 5 commits into from
Oct 17, 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 most AF_UNIX sockets.
    • Fix those tests that forgot to add s2n_io_pair_close().

Call-outs:

  • See PR#4834 and PR#4835 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.
  • Address this comment in PR#4841.

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:46
tests/unit/s2n_alerts_protocol_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_handshake_partial_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_io_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_mem_usage_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_mem_usage_test.c Outdated Show resolved Hide resolved
@boquan-fang boquan-fang changed the title fix: fix opened AF_UNIX sockets that are didn't call s2n_io_pair_close fix: fix opened AF_UNIX sockets that didn't call s2n_io_pair_close Oct 10, 2024
tests/testlib/s2n_connection_test_utils.c Outdated Show resolved Hide resolved
tests/testlib/s2n_testlib.h Outdated Show resolved Hide resolved
* fix all problems and move s2n_connection_test_utilc.c and
  s2n_testlib.h changes to another PR
tests/unit/s2n_mem_usage_test.c Show resolved Hide resolved
tests/unit/s2n_ktls_io_sendfile_test.c Outdated Show resolved Hide resolved
@boquan-fang boquan-fang enabled auto-merge (squash) October 17, 2024 00:53
@boquan-fang boquan-fang merged commit 2587f74 into aws:main Oct 17, 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