Skip to content

Commit 7d129e1

Browse files
committed
Don't leak extra fds into recorded processes
We are pretty good about setting CLOEXEC on most file descriptors. However, we were missing them on the pagemap fd and the tty fd. With lots of nested detaching, this can start filling up fd space, which manifested as a CI failure on julia CI: JuliaLang/julia#58979 Fix this by setting CLOEXEC as appropriate and add a test to make sure this doesn't regress.
1 parent 086bb56 commit 7d129e1

5 files changed

Lines changed: 23 additions & 4 deletions

File tree

CMakeLists.txt

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -716,7 +716,7 @@ if (intel_pt_decoding)
716716
endif()
717717

718718
add_executable(rr ${RR_SOURCES})
719-
target_compile_definitions(rr PRIVATE
719+
target_compile_definitions(rr PRIVATE
720720
"FULL_LIBDIR=\"${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}\"")
721721
set_target_properties(rr PROPERTIES ENABLE_EXPORTS true)
722722
post_build_executable(rr)
@@ -1745,6 +1745,7 @@ set(TESTS_WITHOUT_PROGRAM
17451745
exec_stop
17461746
execp
17471747
explicit_checkpoint_clone
1748+
fd_leak
17481749
file_name_newline
17491750
final_sigkill
17501751
first_instruction
@@ -2020,7 +2021,7 @@ if(BUILD_TESTS)
20202021
bash source_dir/src/test/${test}.run ${testname} -n bin_dir ${TEST_MONITOR_DEFAULT_TIMEOUT})
20212022
configure_test(${test}-no-syscallbuf)
20222023
endforeach(test)
2023-
2024+
20242025
# Run 32-bit tests on 64-bit builds.
20252026
# We copy the test files into '32' subdirectories in the output
20262027
# directory, so we can set different compile options on them.

src/AutoRemoteSyscalls.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,7 @@ template <typename Arch> ScopedFd AutoRemoteSyscalls::retrieve_fd_arch(int fd) {
724724
// pidfd_getfd requires a threadgroup leader, so find one if we can.
725725
Task* tg_leader_for_fds = thread_group_leader_for_fds(t);
726726
if (tg_leader_for_fds) {
727+
// N.B.: pidfd_open fds are always cloexec
727728
pid_fd = ScopedFd(::syscall(NativeArch::pidfd_open, tg_leader_for_fds->tid, 0));
728729
ASSERT(t, pid_fd.is_open() || errno == ENOSYS)
729730
<< "Error in pidfd_open errno=" << errno_name(errno);

src/Task.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2993,7 +2993,7 @@ void Task::open_mem_fd_if_needed() {
29932993

29942994
ScopedFd& Task::pagemap_fd() {
29952995
if (!as->pagemap_fd().is_open()) {
2996-
ScopedFd fd(proc_pagemap_path().c_str(), O_RDONLY);
2996+
ScopedFd fd(proc_pagemap_path().c_str(), O_RDONLY | O_CLOEXEC);
29972997
if (fd.is_open()) {
29982998
as->set_pagemap_fd(std::move(fd));
29992999
} else {

src/record_syscall.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6538,7 +6538,7 @@ static bool is_rr_terminal(const string& pathname) {
65386538
static int dev_tty_fd() {
65396539
static int fd = -1;
65406540
if (fd < 0) {
6541-
fd = open("/dev/tty", O_WRONLY);
6541+
fd = open("/dev/tty", O_WRONLY | O_CLOEXEC);
65426542
}
65436543
return fd;
65446544
}

src/test/fd_leak.run

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
source `dirname $0`/util.sh
2+
num_baseline=$(ls -la /proc/self/fd | wc -l)
3+
just_record $(which rr) "record --nested=detach ls -la /proc/self/fd"
4+
replay
5+
num_replay=$(cat replay.out | wc -l)
6+
# We allow for two extra fds in the recorded process,
7+
# one for the RR fd 999 and one for the perf_events
8+
# fd opened by the syscall buf (if present).
9+
expected_extra_fds=2
10+
if [[ "-n" == "$LIB_ARG" ]]; then
11+
expected_extra_fds=$((expected_extra_fds - 1))
12+
fi
13+
if [[ $num_replay -gt $((num_baseline + expected_extra_fds)) || $num_replay -lt $num_baseline ]]; then
14+
failed
15+
else
16+
passed
17+
fi

0 commit comments

Comments
 (0)