From: Alexandra Hájková Date: Wed, 28 Feb 2024 08:02:15 +0000 (+0100) Subject: With --track-fds=yes warn when file descriptor is closed a second time X-Git-Tag: VALGRIND_3_23_0~119 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6a28e665f8dd7c031aef5aa0eaa4acbbd8ba27a9;p=thirdparty%2Fvalgrind.git With --track-fds=yes warn when file descriptor is closed a second time We moved the record_fd_close call from POST to PRE sys_close handler, because the POST handler is only called on success. Even if the close syscall fails the file descriptor is still really closed/invalid. In the PRE handler the file descriptor is about to be closed, but hasn't been yet so we can capture also the description. This patch add new field fd_closed to OpenFd structure to record if the file descriptor was already closed. We now capture a backtrace when closing file descriptors to be able to print it in a case of a double close. Always add '<' brackets '>' around "unbound" in the description for consistency. getsockdetails now takes and returns a buffer describing the socket because we want to record it, not just print it. Note that close_range is handled similar to closing each descriptor individually. But the case when the close_range is called with an infinite end (~0U) is treated special. Add a new record_fd_close_range function which handles close_range with an infinite end so double close by close_range isn't an error because we don't want to loop over such a wide range. Add a new test cases: - none/tests/socket_close.vgtest - tests double closing a socket - none/tests/double_close_range.vgtest - uses close_range to double close the file descriptors - none/tests/file_dclose.vgtest - double closing regular file with regular close syscall https://bugs.kde.org/show_bug.cgi?id=471222 Co-Authored-By: Mark Wielaard --- diff --git a/NEWS b/NEWS index 0aa3d0eb3..27a6dfa3b 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,10 @@ AMD64/macOS 10.13 and nanoMIPS/Linux. * ==================== CORE CHANGES =================== +* --track-fds=yes will now also warn about double closing of file + descriptors. Printing the context where the file descriptor was + originally opened and where it was previously closed. + * ================== PLATFORM CHANGES ================= * ==================== TOOL CHANGES =================== @@ -36,6 +40,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 466762 Add redirs for C23 free_sized() and free_aligned_sized() 466884 Missing writev uninit padding suppression for _XSend 471036 disInstr_AMD64: disInstr miscalculated next %rip on RORX imm8, m32/64, r32/6 +471222 support tracking of file descriptors being double closed 475498 Add reallocarray wrapper 476320 Build failure with GCC 476331 clean up generated/distributed filter scripts diff --git a/coregrind/m_syswrap/priv_syswrap-generic.h b/coregrind/m_syswrap/priv_syswrap-generic.h index 6c3cd26d4..ec174a152 100644 --- a/coregrind/m_syswrap/priv_syswrap-generic.h +++ b/coregrind/m_syswrap/priv_syswrap-generic.h @@ -59,7 +59,8 @@ extern Bool ML_(fd_allowed)(Int fd, const HChar *syscallname, ThreadId tid, Bool isNewFD); -extern void ML_(record_fd_close) (Int fd); +extern void ML_(record_fd_close) (ThreadId tid, Int fd); +extern void ML_(record_fd_close_range) (ThreadId tid, Int fd); extern void ML_(record_fd_open_named) (ThreadId tid, Int fd); extern void ML_(record_fd_open_nameless) (ThreadId tid, Int fd); extern void ML_(record_fd_open_with_given_name)(ThreadId tid, Int fd, diff --git a/coregrind/m_syswrap/syswrap-amd64-linux.c b/coregrind/m_syswrap/syswrap-amd64-linux.c index 0b751f96d..a59e01826 100644 --- a/coregrind/m_syswrap/syswrap-amd64-linux.c +++ b/coregrind/m_syswrap/syswrap-amd64-linux.c @@ -473,7 +473,7 @@ static SyscallTableEntry syscall_table[] = { GENXY(__NR_read, sys_read), // 0 GENX_(__NR_write, sys_write), // 1 GENXY(__NR_open, sys_open), // 2 - GENXY(__NR_close, sys_close), // 3 + GENX_(__NR_close, sys_close), // 3 GENXY(__NR_stat, sys_newstat), // 4 GENXY(__NR_fstat, sys_newfstat), // 5 diff --git a/coregrind/m_syswrap/syswrap-arm-linux.c b/coregrind/m_syswrap/syswrap-arm-linux.c index ea79c9ba6..217b1c49d 100644 --- a/coregrind/m_syswrap/syswrap-arm-linux.c +++ b/coregrind/m_syswrap/syswrap-arm-linux.c @@ -554,7 +554,7 @@ static SyscallTableEntry syscall_main_table[] = { GENX_(__NR_write, sys_write), // 4 GENXY(__NR_open, sys_open), // 5 - GENXY(__NR_close, sys_close), // 6 + GENX_(__NR_close, sys_close), // 6 // GENXY(__NR_waitpid, sys_waitpid), // 7 GENXY(__NR_creat, sys_creat), // 8 GENX_(__NR_link, sys_link), // 9 diff --git a/coregrind/m_syswrap/syswrap-arm64-linux.c b/coregrind/m_syswrap/syswrap-arm64-linux.c index 61bb4f2d5..ccc548484 100644 --- a/coregrind/m_syswrap/syswrap-arm64-linux.c +++ b/coregrind/m_syswrap/syswrap-arm64-linux.c @@ -605,7 +605,7 @@ static SyscallTableEntry syscall_main_table[] = { LINX_(__NR_fchownat, sys_fchownat), // 54 GENX_(__NR_fchown, sys_fchown), // 55 LINXY(__NR_openat, sys_openat), // 56 - GENXY(__NR_close, sys_close), // 57 + GENX_(__NR_close, sys_close), // 57 LINX_(__NR_vhangup, sys_vhangup), // 58 LINXY(__NR_pipe2, sys_pipe2), // 59 LINX_(__NR_quotactl, sys_quotactl), // 60 diff --git a/coregrind/m_syswrap/syswrap-darwin.c b/coregrind/m_syswrap/syswrap-darwin.c index fd5cb93ce..d6d18ff21 100644 --- a/coregrind/m_syswrap/syswrap-darwin.c +++ b/coregrind/m_syswrap/syswrap-darwin.c @@ -10239,7 +10239,7 @@ const SyscallTableEntry ML_(syscall_table)[] = { GENXY(__NR_read, sys_read), GENX_(__NR_write, sys_write), GENXY(__NR_open, sys_open), - GENXY(__NR_close, sys_close), + GENX_(__NR_close, sys_close), GENXY(__NR_wait4, sys_wait4), _____(VG_DARWIN_SYSCALL_CONSTRUCT_UNIX(8)), // old creat GENX_(__NR_link, sys_link), @@ -10695,7 +10695,7 @@ const SyscallTableEntry ML_(syscall_table)[] = { GENXY(__NR_read_nocancel, sys_read), GENX_(__NR_write_nocancel, sys_write), GENXY(__NR_open_nocancel, sys_open), - GENXY(__NR_close_nocancel, sys_close), + GENX_(__NR_close_nocancel, sys_close), GENXY(__NR_wait4_nocancel, sys_wait4), // 400 MACXY(__NR_recvmsg_nocancel, recvmsg), MACX_(__NR_sendmsg_nocancel, sendmsg), diff --git a/coregrind/m_syswrap/syswrap-freebsd.c b/coregrind/m_syswrap/syswrap-freebsd.c index bbe8286e1..12b3d071d 100644 --- a/coregrind/m_syswrap/syswrap-freebsd.c +++ b/coregrind/m_syswrap/syswrap-freebsd.c @@ -6846,7 +6846,7 @@ POST(sys_close_range) if ((fd != 2/*stderr*/ || VG_(debugLog_getLevel)() == 0) && fd != VG_(log_output_sink).fd && fd != VG_(xml_output_sink).fd) - ML_(record_fd_close)(fd); + ML_(record_fd_close)(tid, fd); } #endif @@ -7119,7 +7119,7 @@ const SyscallTableEntry ML_(syscall_table)[] = { GENX_(__NR_write, sys_write), // 4 GENXY(__NR_open, sys_open), // 5 - GENXY(__NR_close, sys_close), // 6 + GENX_(__NR_close, sys_close), // 6 GENXY(__NR_wait4, sys_wait4), // 7 // 4.3 creat 8 diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index 8f94b172f..4b59fc39b 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -65,6 +65,9 @@ #include "config.h" +static +HChar *getsockdetails(Int fd, UInt len, HChar *buf); + void ML_(guess_and_register_stack) (Addr sp, ThreadState* tst) { Bool debug = False; @@ -538,6 +541,8 @@ typedef struct OpenFd Int fd; /* The file descriptor */ HChar *pathname; /* NULL if not a regular file or unknown */ ExeContext *where; /* NULL if inherited from parent */ + ExeContext *where_closed; /* record the last close of fd */ + Bool fd_closed; struct OpenFd *next, *prev; } OpenFd; @@ -547,9 +552,38 @@ static OpenFd *allocated_fds = NULL; /* Count of open file descriptors. */ static Int fd_count = 0; +/* Close_range caller might want to close very wide range of file descriptors, + up to 0U. We want to avoid iterating through such a range in a normall + close_range, just up to any open file descriptor. Also, unlike + record_fd_close_range, we assume the user might deliberately double closes + any file descriptors in the range, so don't warn about double close here. */ +void ML_(record_fd_close_range)(ThreadId tid, Int from_fd) +{ + OpenFd *i = allocated_fds; + + if (from_fd >= VG_(fd_hard_limit)) + return; /* Valgrind internal */ + + while(i) { + // Assume the user doesn't want a warning if this came from + // close_range. Just record the file descriptors not yet closed here. + if (i->fd >= from_fd && !i->fd_closed + && i->fd != VG_(log_output_sink).fd + && i->fd != VG_(xml_output_sink).fd) { + i->fd_closed = True; + i->where_closed = ((tid == -1) + ? NULL + : VG_(record_ExeContext)(tid, + 0/*first_ip_delta*/)); + fd_count--; + } + i = i->next; + } +} + /* Note the fact that a file descriptor was just closed. */ -void ML_(record_fd_close)(Int fd) +void ML_(record_fd_close)(ThreadId tid, Int fd) { OpenFd *i = allocated_fds; @@ -557,18 +591,47 @@ void ML_(record_fd_close)(Int fd) return; /* Valgrind internal */ while(i) { - if(i->fd == fd) { - if(i->prev) - i->prev->next = i->next; - else - allocated_fds = i->next; - if(i->next) - i->next->prev = i->prev; - if(i->pathname) - VG_(free) (i->pathname); - VG_(free) (i); - fd_count--; - break; + if (i->fd == fd) { + if (i->fd_closed) { + char *path = i->pathname; + // Note we might want to turn this into a "Core error" + // Or use pub_core_execontext later. + // VG_(print_ExeContext_stats) (True ...); + VG_(emit)("File descriptor %d: %s is already closed\n", + fd, path); + VG_(pp_ExeContext)(VG_(record_ExeContext)(tid, 0)); + VG_(emit)(" Previously closed\n"); + VG_(pp_ExeContext)(i->where_closed); + VG_(emit)(" Originally opened\n"); + VG_(pp_ExeContext)(i->where); + } else { + i->fd_closed = True; + i->where_closed = ((tid == -1) + ? NULL + : VG_(record_ExeContext)(tid, + 0/*first_ip_delta*/)); + /* Record path/socket name, etc. In case we want to print it later, + for example for double close. Note that record_fd_close is + actually called from the PRE syscall handler, so the file + description is about to be closed, but hasn't yet at this + point. */ + if (!i->pathname) { + Int val; + Int len = sizeof(val); + if (VG_(getsockopt)(i->fd, VKI_SOL_SOCKET, VKI_SO_TYPE, + &val, &len) == -1) { + VG_(printf)("not sockopt: %d\n", i->fd); + HChar *pathname = VG_(malloc)("vg.record_fd_close.fd", 30); + VG_(snprintf)(pathname, 30, "file descriptor %d\n", i->fd); + i->pathname = pathname; + } else { + HChar *name = VG_(malloc)("vg.record_fd_close.sock", 256); + i->pathname = getsockdetails(i->fd, 256, name); + } + } + fd_count--; + } + break; } i = i->next; } @@ -588,11 +651,17 @@ void ML_(record_fd_open_with_given_name)(ThreadId tid, Int fd, if (fd >= VG_(fd_hard_limit)) return; /* Valgrind internal */ - /* Check to see if this fd is already open. */ + /* Check to see if this fd is already open (or closed, we will just + override it. */ i = allocated_fds; while (i) { if (i->fd == fd) { - if (i->pathname) VG_(free)(i->pathname); + if (i->pathname) { + VG_(free)(i->pathname); + i->pathname = NULL; + } + if (i->fd_closed) /* now we will open it. */ + fd_count++; break; } i = i->next; @@ -612,6 +681,8 @@ void ML_(record_fd_open_with_given_name)(ThreadId tid, Int fd, i->fd = fd; i->pathname = VG_(strdup)("syswrap.rfdowgn.2", pathname); i->where = (tid == -1) ? NULL : VG_(record_ExeContext)(tid, 0/*first_ip_delta*/); + i->fd_closed = False; + i->where_closed = NULL; } // Record opening of an fd, and find its name. @@ -638,8 +709,12 @@ Bool ML_(fd_recorded)(Int fd) { OpenFd *i = allocated_fds; while (i) { - if (i->fd == fd) - return True; + if (i->fd == fd) { + if (i->fd_closed) + return False; + else + return True; + } i = i->next; } return False; @@ -651,8 +726,12 @@ const HChar *ML_(find_fd_recorded_by_fd)(Int fd) OpenFd *i = allocated_fds; while (i) { - if (i->fd == fd) - return i->pathname; + if (i->fd == fd) { + if (i->fd_closed) + return NULL; + else + return i->pathname; + } i = i->next; } @@ -754,9 +833,10 @@ HChar *inet6_to_name(struct vki_sockaddr_in6 *sa, UInt len, HChar *name) /* * Try get some details about a socket. + * Returns the given BUF with max length LEN. */ -static void -getsockdetails(Int fd) +static +HChar *getsockdetails(Int fd, UInt len, HChar *buf) { union u { struct vki_sockaddr a; @@ -778,15 +858,16 @@ getsockdetails(Int fd) Int plen = sizeof(struct vki_sockaddr_in); if (VG_(getpeername)(fd, (struct vki_sockaddr *)&paddr, &plen) != -1) { - VG_(message)(Vg_UserMsg, "Open AF_INET socket %d: %s <-> %s\n", fd, - inet_to_name(&(laddr.in), llen, lname), - inet_to_name(&paddr, plen, pname)); + VG_(snprintf)(buf, len, "AF_INET socket %d: %s <-> %s", fd, + inet_to_name(&(laddr.in), llen, lname), + inet_to_name(&paddr, plen, pname)); + return buf; } else { - VG_(message)(Vg_UserMsg, "Open AF_INET socket %d: %s <-> unbound\n", - fd, inet_to_name(&(laddr.in), llen, lname)); - } - return; + VG_(snprintf)(buf, len, "AF_INET socket %d: %s <-> ", + fd, inet_to_name(&(laddr.in), llen, lname)); + return buf; } + } case VKI_AF_INET6: { HChar lname[128]; // large enough HChar pname[128]; // large enough @@ -794,29 +875,31 @@ getsockdetails(Int fd) Int plen = sizeof(struct vki_sockaddr_in6); if (VG_(getpeername)(fd, (struct vki_sockaddr *)&paddr, &plen) != -1) { - VG_(message)(Vg_UserMsg, "Open AF_INET6 socket %d: %s <-> %s\n", fd, - inet6_to_name(&(laddr.in6), llen, lname), - inet6_to_name(&paddr, plen, pname)); + VG_(snprintf)(buf, len, "AF_INET6 socket %d: %s <-> %s", fd, + inet6_to_name(&(laddr.in6), llen, lname), + inet6_to_name(&paddr, plen, pname)); + return buf; } else { - VG_(message)(Vg_UserMsg, "Open AF_INET6 socket %d: %s <-> unbound\n", + VG_(snprintf)(buf, len, "AF_INET6 socket %d: %s <-> ", fd, inet6_to_name(&(laddr.in6), llen, lname)); + return buf; } - return; - } + } case VKI_AF_UNIX: { static char lname[256]; - VG_(message)(Vg_UserMsg, "Open AF_UNIX socket %d: %s\n", fd, - unix_to_name(&(laddr.un), llen, lname)); - return; + VG_(snprintf)(buf, len, "AF_UNIX socket %d: %s", fd, + unix_to_name(&(laddr.un), llen, lname)); + return buf; } default: - VG_(message)(Vg_UserMsg, "Open pf-%d socket %d:\n", - laddr.a.sa_family, fd); - return; + VG_(snprintf)(buf, len, "pf-%d socket %d", + laddr.a.sa_family, fd); + return buf; } } - VG_(message)(Vg_UserMsg, "Open socket %d:\n", fd); + VG_(snprintf)(buf, len, "socket %d", fd); + return buf; } @@ -827,7 +910,7 @@ void VG_(show_open_fds) (const HChar* when) int non_std = 0; for (i = allocated_fds; i; i = i->next) { - if (i->fd > 2) + if (i->fd > 2 && i->fd_closed != True) non_std++; } @@ -842,6 +925,9 @@ void VG_(show_open_fds) (const HChar* when) fd_count, fd_count - non_std, when); for (i = allocated_fds; i; i = i->next) { + if (i->fd_closed) + continue; + if (i->fd <= 2 && VG_(clo_track_fds) < 2) continue; @@ -856,7 +942,9 @@ void VG_(show_open_fds) (const HChar* when) == -1) { VG_(message)(Vg_UserMsg, "Open file descriptor %d:\n", i->fd); } else { - getsockdetails(i->fd); + HChar buf[256]; + VG_(message)(Vg_UserMsg, "Open %s\n", + getsockdetails(i->fd, 256, buf)); } } @@ -1434,7 +1522,7 @@ Bool ML_(fd_allowed)(Int fd, const HChar *syscallname, ThreadId tid, /* croak? */ if ((!allowed) && VG_(showing_core_errors)() ) { - VG_(message)(Vg_UserMsg, + VG_(message)(Vg_UserMsg, "Warning: invalid file descriptor %d in syscall %s()\n", fd, syscallname); if (fd == VG_(log_output_sink).fd && VG_(log_output_sink).fd >= 0) @@ -3368,11 +3456,13 @@ PRE(sys_close) allow that to be closed either. */ || (ARG1 == 2/*stderr*/ && VG_(debugLog_getLevel)() > 0) ) SET_STATUS_Failure( VKI_EBADF ); -} - -POST(sys_close) -{ - if (VG_(clo_track_fds)) ML_(record_fd_close)(ARG1); + else { + /* We used to do close tracking in the POST handler, but that is + only called on success. Even if the close syscall fails the + file descriptor is still really closed/invalid. So we do the + recording and checking here. */ + if (VG_(clo_track_fds)) ML_(record_fd_close)(tid, ARG1); + } } PRE(sys_dup) diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c index 01bbba0a6..45413fdd9 100644 --- a/coregrind/m_syswrap/syswrap-linux.c +++ b/coregrind/m_syswrap/syswrap-linux.c @@ -13540,11 +13540,17 @@ POST(sys_close_range) if (last >= VG_(fd_hard_limit)) last = VG_(fd_hard_limit) - 1; - for (fd = ARG1; fd <= last; fd++) - if ((fd != 2/*stderr*/ || VG_(debugLog_getLevel)() == 0) - && fd != VG_(log_output_sink).fd - && fd != VG_(xml_output_sink).fd) - ML_(record_fd_close)(fd); + /* If the close_range range is too wide, we don't want to loop + through the whole range. */ + if (ARG2 == ~0U) + ML_(record_fd_close_range)(tid, ARG1); + else { + for (fd = ARG1; fd <= last; fd++) + if ((fd != 2/*stderr*/ || VG_(debugLog_getLevel)() == 0) + && fd != VG_(log_output_sink).fd + && fd != VG_(xml_output_sink).fd) + ML_(record_fd_close)(tid, fd); + } } diff --git a/coregrind/m_syswrap/syswrap-mips32-linux.c b/coregrind/m_syswrap/syswrap-mips32-linux.c index 5f7e2603f..3f991da0a 100644 --- a/coregrind/m_syswrap/syswrap-mips32-linux.c +++ b/coregrind/m_syswrap/syswrap-mips32-linux.c @@ -771,7 +771,7 @@ static SyscallTableEntry syscall_main_table[] = { GENXY (__NR_read, sys_read), // 3 GENX_ (__NR_write, sys_write), // 4 GENXY (__NR_open, sys_open), // 5 - GENXY (__NR_close, sys_close), // 6 + GENX_ (__NR_close, sys_close), // 6 GENXY (__NR_waitpid, sys_waitpid), // 7 GENXY (__NR_creat, sys_creat), // 8 GENX_ (__NR_link, sys_link), // 9 diff --git a/coregrind/m_syswrap/syswrap-mips64-linux.c b/coregrind/m_syswrap/syswrap-mips64-linux.c index f9af1300d..9899a21cf 100644 --- a/coregrind/m_syswrap/syswrap-mips64-linux.c +++ b/coregrind/m_syswrap/syswrap-mips64-linux.c @@ -513,7 +513,7 @@ static SyscallTableEntry syscall_main_table[] = { GENXY (__NR_read, sys_read), /* 5000 */ GENX_ (__NR_write, sys_write), GENXY (__NR_open, sys_open), - GENXY (__NR_close, sys_close), + GENX_ (__NR_close, sys_close), GENXY (__NR_stat, sys_newstat), GENXY (__NR_fstat, sys_newfstat), GENXY (__NR_lstat, sys_newlstat), diff --git a/coregrind/m_syswrap/syswrap-nanomips-linux.c b/coregrind/m_syswrap/syswrap-nanomips-linux.c index 34913a256..dc99f3d55 100644 --- a/coregrind/m_syswrap/syswrap-nanomips-linux.c +++ b/coregrind/m_syswrap/syswrap-nanomips-linux.c @@ -610,7 +610,7 @@ static SyscallTableEntry syscall_main_table[] = { LINX_ (__NR_fchownat, sys_fchownat), GENX_ (__NR_fchown, sys_fchown), LINXY (__NR_openat, sys_openat), - GENXY (__NR_close, sys_close), + GENX_ (__NR_close, sys_close), LINX_ (__NR_vhangup, sys_vhangup), LINXY (__NR_pipe2, sys_pipe2), LINX_ (__NR_quotactl, sys_quotactl), diff --git a/coregrind/m_syswrap/syswrap-ppc32-linux.c b/coregrind/m_syswrap/syswrap-ppc32-linux.c index 83e9e72eb..0aabfbb17 100644 --- a/coregrind/m_syswrap/syswrap-ppc32-linux.c +++ b/coregrind/m_syswrap/syswrap-ppc32-linux.c @@ -618,7 +618,7 @@ static SyscallTableEntry syscall_table[] = { GENX_(__NR_write, sys_write), // 4 GENXY(__NR_open, sys_open), // 5 - GENXY(__NR_close, sys_close), // 6 + GENX_(__NR_close, sys_close), // 6 GENXY(__NR_waitpid, sys_waitpid), // 7 GENXY(__NR_creat, sys_creat), // 8 GENX_(__NR_link, sys_link), // 9 diff --git a/coregrind/m_syswrap/syswrap-ppc64-linux.c b/coregrind/m_syswrap/syswrap-ppc64-linux.c index aeffbe072..35e3f8ec4 100644 --- a/coregrind/m_syswrap/syswrap-ppc64-linux.c +++ b/coregrind/m_syswrap/syswrap-ppc64-linux.c @@ -607,7 +607,7 @@ static SyscallTableEntry syscall_table[] = { GENX_(__NR_write, sys_write), // 4 GENXY(__NR_open, sys_open), // 5 - GENXY(__NR_close, sys_close), // 6 + GENX_(__NR_close, sys_close), // 6 GENXY(__NR_waitpid, sys_waitpid), // 7 GENXY(__NR_creat, sys_creat), // 8 GENX_(__NR_link, sys_link), // 9 diff --git a/coregrind/m_syswrap/syswrap-s390x-linux.c b/coregrind/m_syswrap/syswrap-s390x-linux.c index b720f62d0..f941bdd19 100644 --- a/coregrind/m_syswrap/syswrap-s390x-linux.c +++ b/coregrind/m_syswrap/syswrap-s390x-linux.c @@ -418,7 +418,7 @@ static SyscallTableEntry syscall_table[] = { GENX_(__NR_write, sys_write), // 4 GENXY(__NR_open, sys_open), // 5 - GENXY(__NR_close, sys_close), // 6 + GENX_(__NR_close, sys_close), // 6 // ?????(__NR_restart_syscall, ), // 7 GENXY(__NR_creat, sys_creat), // 8 GENX_(__NR_link, sys_link), // 9 diff --git a/coregrind/m_syswrap/syswrap-solaris.c b/coregrind/m_syswrap/syswrap-solaris.c index 5bd049849..8e60ebc98 100644 --- a/coregrind/m_syswrap/syswrap-solaris.c +++ b/coregrind/m_syswrap/syswrap-solaris.c @@ -1791,7 +1791,7 @@ POST(sys_close) /* Possibly an explicitly open'ed client door fd was just closed. Generic sys_close wrapper calls this only if VG_(clo_track_fds) = True. */ if (!VG_(clo_track_fds)) - ML_(record_fd_close)(ARG1); + ML_(record_fd_close)(tid, ARG1); } PRE(sys_linkat) @@ -8693,7 +8693,7 @@ static Int pre_check_and_close_fds(ThreadId tid, const HChar *name, if ((desc->d_attributes & DOOR_DESCRIPTOR) && (desc->d_attributes & DOOR_RELEASE)) { Int fd = desc->d_data.d_desc.d_descriptor; - ML_(record_fd_close)(fd); + ML_(record_fd_close)(tid, fd); } } } @@ -9563,7 +9563,7 @@ POST(sys_door) case VKI_DOOR_REVOKE: door_record_revoke(tid, ARG1); if (VG_(clo_track_fds)) - ML_(record_fd_close)(ARG1); + ML_(record_fd_close)(tid, ARG1); break; case VKI_DOOR_INFO: POST_MEM_WRITE(ARG2, sizeof(vki_door_info_t)); diff --git a/coregrind/m_syswrap/syswrap-x86-linux.c b/coregrind/m_syswrap/syswrap-x86-linux.c index 9afd03695..c6cf682e7 100644 --- a/coregrind/m_syswrap/syswrap-x86-linux.c +++ b/coregrind/m_syswrap/syswrap-x86-linux.c @@ -1163,7 +1163,7 @@ static SyscallTableEntry syscall_table[] = { GENX_(__NR_write, sys_write), // 4 GENXY(__NR_open, sys_open), // 5 - GENXY(__NR_close, sys_close), // 6 + GENX_(__NR_close, sys_close), // 6 GENXY(__NR_waitpid, sys_waitpid), // 7 GENXY(__NR_creat, sys_creat), // 8 GENX_(__NR_link, sys_link), // 9 diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am index e13a0493b..a37bb2725 100644 --- a/none/tests/Makefile.am +++ b/none/tests/Makefile.am @@ -226,7 +226,10 @@ EXTRA_DIST = \ vgprintf.stderr.exp vgprintf.vgtest \ vgprintf_nvalgrind.stderr.exp vgprintf_nvalgrind.vgtest \ process_vm_readv_writev.stderr.exp process_vm_readv_writev.vgtest \ - sigprocmask.stderr.exp sigprocmask.vgtest + sigprocmask.stderr.exp sigprocmask.vgtest \ + socket_close.stderr.exp socket_close.vgtest \ + file_dclose.stderr.exp file_dclose.vgtest \ + double_close_range.stderr.exp double_close_range.vgtest check_PROGRAMS = \ @@ -277,7 +280,13 @@ check_PROGRAMS = \ coolo_sigaction \ gxx304 \ process_vm_readv_writev \ - sigprocmask + sigprocmask \ + socket_close \ + file_dclose + +if HAVE_CLOSE_RANGE + check_PROGRAMS += double_close_range +endif if HAVE_NESTED_FUNCTIONS check_PROGRAMS += nestedfns diff --git a/none/tests/double_close_range.c b/none/tests/double_close_range.c new file mode 100644 index 000000000..623959330 --- /dev/null +++ b/none/tests/double_close_range.c @@ -0,0 +1,27 @@ +#define _GNU_SOURCE +#include +#include +#include +#include +#if defined(__linux__) +#include +#endif + +int main () +{ + int fd = dup (2); + if (fd != -1){ + fprintf(stderr, "close fd %d\n", fd); + close (fd); + } + + // Shouldn't warn, we are closing everything + fprintf(stderr, "Closing range (%d, %d).\n", fd, ~0U); + close_range(6, ~0U, 0); + + // Should warn, we close a small range (but only for fd itself + fprintf(stderr, "Closing range (%d, %d).\n", fd, 7); + close_range(5, 7, 0); + + return 0; +} diff --git a/none/tests/double_close_range.stderr.exp b/none/tests/double_close_range.stderr.exp new file mode 100644 index 000000000..f1b78ea1d --- /dev/null +++ b/none/tests/double_close_range.stderr.exp @@ -0,0 +1,3 @@ +close fd 3 +Closing range (3, -1). +Closing range (3, 7). diff --git a/none/tests/double_close_range.vgtest b/none/tests/double_close_range.vgtest new file mode 100644 index 000000000..8320d03f9 --- /dev/null +++ b/none/tests/double_close_range.vgtest @@ -0,0 +1,3 @@ +prog: double_close_range +prereq: test -x double_close_range +vgopts: -q --track-fds=yes diff --git a/none/tests/fdleak.h b/none/tests/fdleak.h index c94dbde31..6695138a4 100644 --- a/none/tests/fdleak.h +++ b/none/tests/fdleak.h @@ -1,9 +1,11 @@ #ifndef _FDLEAK_H_ #define _FDLEAK_H_ +#define _GNU_SOURCE #include #include #include +#include #define DO(op) \ ({ \ @@ -25,14 +27,18 @@ * https://bugs.launchpad.net/ubuntu/+source/seahorse/+bug/235184 */ static inline void close_inherited () { + struct stat sb; int i; int max_fds = sysconf (_SC_OPEN_MAX); if (max_fds < 0) max_fds = 1024; /* Fallback if sysconf fails, returns -1. */ /* Only leave 0 (stdin), 1 (stdout) and 2 (stderr) open. */ for (i = 3; i < max_fds; i++) - close(i); + if (fstat (i, &sb) != -1) /* Test if the file descriptor exists first. */ + close(i); } #define CLOSE_INHERITED_FDS close_inherited () +/* Note that the following would be nicer, but close_range is fairly new. */ +// #define CLOSE_INHERITED_FDS close_range (3, ~0U, 0) #endif /* _FDLEAK_H_ */ diff --git a/none/tests/fdleak_ipv4.c b/none/tests/fdleak_ipv4.c index 89f7acb9c..05f297762 100644 --- a/none/tests/fdleak_ipv4.c +++ b/none/tests/fdleak_ipv4.c @@ -63,9 +63,16 @@ void client () exit(1); } + // Extra double close test... + // Just to see that the description is OK. + int fd2 = DO( dup (s) ); + close (fd2); + close (fd2); // oops. + (void) DO( read(s, buf, sizeof(buf)) ); printf("%s\n", buf); + } diff --git a/none/tests/fdleak_ipv4.stderr.exp b/none/tests/fdleak_ipv4.stderr.exp index 72c2b4685..2ca7c291e 100644 --- a/none/tests/fdleak_ipv4.stderr.exp +++ b/none/tests/fdleak_ipv4.stderr.exp @@ -1,10 +1,22 @@ +File descriptor 4: AF_INET socket 4: 127.0.0.1:... <-> 127.0.0.1:... is already closed + at 0x........: close (in /...libc...) + by 0x........: client (fdleak_ipv4.c:70) + by 0x........: main (fdleak_ipv4.c:90) + Previously closed + at 0x........: close (in /...libc...) + by 0x........: client (fdleak_ipv4.c:69) + by 0x........: main (fdleak_ipv4.c:90) + Originally opened + at 0x........: dup (in /...libc...) + by 0x........: client (fdleak_ipv4.c:68) + by 0x........: main (fdleak_ipv4.c:90) FILE DESCRIPTORS: 5 open (3 std) at exit. Open AF_INET socket 4: 127.0.0.1:... <-> 127.0.0.1:... ... -Open AF_INET socket 3: 127.0.0.1:... <-> unbound +Open AF_INET socket 3: 127.0.0.1:... <-> ... diff --git a/none/tests/file_dclose.c b/none/tests/file_dclose.c new file mode 100644 index 000000000..8856fa670 --- /dev/null +++ b/none/tests/file_dclose.c @@ -0,0 +1,35 @@ +#define _GNU_SOURCE +#include +#include +#include +#include + +static int +openfile (const char *f) +{ + return creat (f, O_RDWR); +} + +static void +closefile (const char *f, int fd) +{ + close (fd); + unlink (f); +} + +int main () +{ + const char *TMPFILE = "file_dclose.tmp"; + int fd; + + fd = openfile (TMPFILE); + if (fd != -1) { + fprintf(stderr, "close %d\n", fd); + close (fd); + } + + fprintf (stderr, "time passes and we close %d again\n", fd); + closefile (TMPFILE, fd); + + return 0; +} diff --git a/none/tests/file_dclose.stderr.exp b/none/tests/file_dclose.stderr.exp new file mode 100644 index 000000000..d27648d2d --- /dev/null +++ b/none/tests/file_dclose.stderr.exp @@ -0,0 +1,13 @@ +close 3 +time passes and we close 3 again +File descriptor 3: file_dclose.tmp is already closed + at 0x........: close (in /...libc...) + by 0x........: closefile (file_dclose.c:16) + by 0x........: main (file_dclose.c:32) + Previously closed + at 0x........: close (in /...libc...) + by 0x........: main (file_dclose.c:28) + Originally opened + at 0x........: creat (in /...libc...) + by 0x........: openfile (file_dclose.c:10) + by 0x........: main (file_dclose.c:25) diff --git a/none/tests/file_dclose.vgtest b/none/tests/file_dclose.vgtest new file mode 100644 index 000000000..3f13a1dc3 --- /dev/null +++ b/none/tests/file_dclose.vgtest @@ -0,0 +1,3 @@ +prog: file_dclose +prereq: test -x file_dclose +vgopts: -q --track-fds=yes diff --git a/none/tests/socket_close.c b/none/tests/socket_close.c new file mode 100644 index 000000000..99d277229 --- /dev/null +++ b/none/tests/socket_close.c @@ -0,0 +1,41 @@ +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include + + +int socket_fd; + +void *open_socket() +{ + socket_fd = socket(AF_UNIX, SOCK_STREAM, 0); + fprintf (stderr, "Open socket %d\n", socket_fd); + struct sockaddr_un my_addr; + + memset(&my_addr, 0, sizeof(my_addr)); + my_addr.sun_family = AF_UNIX; + strncpy(my_addr.sun_path, "/tmp/foofrob", sizeof(my_addr.sun_path) - 1); + bind(socket_fd, (struct sockaddr *) &my_addr, sizeof(my_addr)); + + return NULL; +} + +int main () +{ + open_socket(); + + if (socket_fd != -1) + { + fprintf(stderr, "close socket_fd %d\n", socket_fd); + close (socket_fd); + } + + fprintf (stderr, "and close the socket again %d\n", socket_fd); + close (socket_fd); + + return 0; +} + diff --git a/none/tests/socket_close.stderr.exp b/none/tests/socket_close.stderr.exp new file mode 100644 index 000000000..bec4d6370 --- /dev/null +++ b/none/tests/socket_close.stderr.exp @@ -0,0 +1,13 @@ +Open socket 3 +close socket_fd 3 +and close the socket again 3 +File descriptor 3: AF_UNIX socket 3: is already closed + at 0x........: close (in /...libc...) + by 0x........: main (socket_close.c:37) + Previously closed + at 0x........: close (in /...libc...) + by 0x........: main (socket_close.c:33) + Originally opened + at 0x........: socket (in /...libc...) + by 0x........: open_socket (socket_close.c:14) + by 0x........: main (socket_close.c:28) diff --git a/none/tests/socket_close.vgtest b/none/tests/socket_close.vgtest new file mode 100644 index 000000000..118e9da3b --- /dev/null +++ b/none/tests/socket_close.vgtest @@ -0,0 +1,3 @@ +prog: socket_close +prereq: test -x socket_close +vgopts: -q --track-fds=yes