In coregrind/m_syswrap/syswrap-generic.c ML_(fd_allowed) we currently have a comment /* croak? */ after which in some situations we generate warning messages. These should be turned into real errors like those we generate in ML_(record_fd_close). There are three kinds of errors to report when !isNewFd: 1. When !allowed it is an bad file descriptor (either negative or a private one valgrind might be using) 2. When not found in allocated_fds it was an fd number never opened by the program. 3. When found in allocated_fds, but the closed flag is set, the fd was earlier closed. In the last case we should have an where (opened) and a where_closed that we can attach to the error message.
Created attachment 174614 [details] patch
Very nice. This implements errors of type 1. Pushed as: commit ff96f07bc9b97d15bbb05b470f06d46d52c190be Author: Alexandra Hájková <ahajkova@redhat.com> Date: Tue Jun 25 11:27:16 2024 -0400 Add BadFdExtra core error. Introduce a new FdBadFd type with associated extra info struct. Which for now just holds the fd number (no path or description). fd_pp_Error and fd_update_extra have been updated to handle the new type and produce xml when requested. Rename showing_core_errors to showing_core_warning (returns yes when the tools wants to show core errors, -q isn't given and we aren't producing xml). In ML_(fd_allowed) we now call VG_(maybe_record_error) to generate a real error (that can be suppressed and shows up in the xml output with full execution backtrace). For now we also produce the legacy warnings when --track-fds=yes isn't given. Add none/tests/fdbaduse.vgtest to test the new FdBadUse core error. This is the first part of reporting bad fd usage errors. We are also tracking already closed file descriptors which should also produce errors like these. The current bad file descriptors are just those that are negative or above the current file limit of the process. https://bugs.kde.org/show_bug.cgi?id=493418 Keeping this bug open for a followup to implement type 2 and 3 events.
Created attachment 174899 [details] patch Warn when fd number never opened by the program
(In reply to Alexandra Hajkova from comment #3) > Created attachment 174899 [details] > patch > > Warn when fd number never opened by the program Yes, looks OKish. But ML_(find_fd_recorded_by_fd) only works for never created fds (error kind 2) and might give a false positive if there isn't a pathname associated with the OpenFd. Better to have a dedicated function to search for any OpenFd and check the closed flag explicitly. Then you can report on both error kind 2 and 3. For created, then destroyed fds you need the pathname, description, where_opened, where_closed info anyway.
Created attachment 175242 [details] patch
Looks good, thanks. Small tweak to the new testcase and code for some corner cases diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index e7c9c40e5..175a89199 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -1675,7 +1675,8 @@ Bool ML_(fd_allowed)(Int fd, const HChar *syscallname, ThreadId tid, client is exactly what we don't want. */ /* croak? */ - if (!isNewFd && (VG_(strcmp)("close", syscallname) != 0)) { + if (VG_(clo_track_fds) && allowed + && !isNewFd && (VG_(strcmp)("close", syscallname) != 0)) { const OpenFd *openbadfd = ML_(find_OpenFd)(fd); if (!openbadfd) { /* File descriptor which was never created (or inherited). */ diff --git a/none/tests/use_after_close.c b/none/tests/use_after_close.c index 888a76a60..deb4e6888 100644 --- a/none/tests/use_after_close.c +++ b/none/tests/use_after_close.c @@ -8,11 +8,24 @@ int main(void) char *string = "bad\n"; int fd = dup(2); - write(fd, string, 3); + /* OK. */ + write(fd, string, 4); close(fd); - write(fd, string, 3); - write(7, string, 3); + /* Already closed. */ + write(fd, string, 4); + + /* Never created. */ + write(7, string, 4); + + /* Invalid. */ + write(-7, string, 4); + + /* Double double close. */ + close(fd); + + /* Invalid close. */ + close (-7); return 0; } diff --git a/none/tests/use_after_close.stderr.exp b/none/tests/use_after_close.stderr.exp index d3c6fa456..1ef31c655 100644 --- a/none/tests/use_after_close.stderr.exp +++ b/none/tests/use_after_close.stderr.exp @@ -1,4 +1,5 @@ -badFile descriptor 3 was closed already +bad +File descriptor 3 was closed already Previously closed at 0x........: close (in /...libc...) by 0x........: main @@ -10,3 +11,18 @@ badFile descriptor 3 was closed already File descriptor 7 was never created at 0x........: write (in /...libc...) by 0x........: main +File descriptor -7 Invalid file descriptor + at 0x........: write (in /...libc...) + by 0x........: main +File descriptor ...: ... is already closed + at 0x........: close (in /...libc...) + by 0x........: main + Previously closed + at 0x........: close (in /...libc...) + by 0x........: main + Originally opened + at 0x........: dup (in /...libc...) + by 0x........: main +File descriptor -7 Invalid file descriptor + at 0x........: close (in /...libc...) + by 0x........: main
commit 22971a15d62df6351ab97ea064eebd9bdcb4cf37 Author: Alexandra Hájková <ahajkova@redhat.com> Date: Wed Oct 16 13:38:48 2024 -0400 Report track-fd errors for fd used which was not opened or already closed Add (optional) pathname, description, where_closed and where_opened fields to struct FdBadUse. Print those fields when set in fd_pp_Error. Add a new function ML_(find_OpenFd) that provides a recorded OpenFd given an fd (or NULL when the fd was never recorded). In ML_(fd_allowed) when using a file descriptor use ML_(find_OpenFd) to see if the fd was ever created, if not create an "was never created" FdBadUse error. If it was created, but already closed create an "was closed already", filling in as much details as we can. Add none/tests/use_after_close.vgtest to test, already closed, never created, invalid, double (double) close and invalid close issues. Adjust error message in none/tests/fdbaduse.stderr.exp. https://bugs.kde.org/show_bug.cgi?id=493418