https://bugs.kde.org/show_bug.cgi?id=331311
--- Comment #7 from Mark Wielaard <[email protected]> --- (In reply to Alexandra Hajkova from comment #6) > This change prevents client programs from seeing Valgrind's internal file > descriptors when scanning /proc/self/fd or /proc/<pid>/fd, Nice. > addressing a > security > and compatibility issue where defensive programs that close all > non-whitelisted > file descriptors would crash when attempting to close Valgrind's reserved > descriptors. That is overstating a bit imho. Just keep it short and simple with that first sentence. > This patch modifies the getdents and getdents64 syscall wrappers to > selectively > filter out Valgrind's internal file descriptors only when listing > /proc/*/fd > directories. OK, maybe add "for the current process"? > Regular directory listings remain unaffected, ensuring the > fix is > targeted and doesn't interfere with normal filesystem operations. Yes, of course, doesn't really need to be stated imho. > Add none/tests/getdents_filter.vgtest test that tests that the > Valgrind's file descriptors are hiddent from the client program > and verifies both /proc/self/fd filtering and that regular > directory listings remain unfiltered. OK. BTW. These sentences/lines are a bit wide, please keep < 78 chars if possible. > https://bugs.kde.org/show_bug.cgi?id=331311 Please also add this to NEWS. > diff --git a/.gitignore b/.gitignore > index e48a2ab0e..86a028cd6 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -1687,6 +1687,7 @@ > /none/tests/vgprintf > /none/tests/vgprintf_nvalgrind > /none/tests/yield > +/none/tests/getdents_filter OK > +/* Check if the given file descriptor points to a /proc/PID/fd directory. > + Returns True if it's a directory we should filter Valgrind FDs from. */ > +static Bool is_proc_fd_directory(Int fd) > +{ > + const HChar* path; > + if (!VG_(resolve_filename)(fd, &path)) > + return False; OK, that works and doesn't leak because path will point to a static buffer inside resolve_filename. Not thread safe, but OK because we are single threaded here. > + /* Check for /proc/self/fd or /proc/PID/fd patterns */ > + if (VG_(strncmp)(path, "/proc/", 6) != 0) > + return False; > + > + /* Look for "/fd" at the end */ > + Int path_len = VG_(strlen)(path); > + if (path_len < 9) /* Minimum: /proc/1/fd */ > + return False; > + > + if (VG_(strcmp)(path + path_len - 3, "/fd") != 0) > + return False; > + > + /* Check if it's /proc/self/fd */ > + if (VG_(strncmp)(path + 6, "self/fd", 7) == 0) > + return True; > + > + /* Check if it's /proc/<valgrind_pid>/fd */ > + Int valgrind_pid = VG_(getpid)(); > + HChar pid_str[16]; > + VG_(sprintf)(pid_str, "%d", valgrind_pid); > + Int pid_str_len = VG_(strlen)(pid_str); > + > + if (VG_(strncmp)(path + 6, pid_str, pid_str_len) == 0 && > + VG_(strcmp)(path + 6 + pid_str_len, "/fd") == 0) > + return True; That is a lot of checks, and probably correct, but why not do something a bit simpler like: HChar ppfd_name[26] /* large enough 6 + 16 + 4 */ VG_(sprintf)(ppfd, "/proc/%d/fd", VG_(getpid)()); if (VG_(strcmp)(path, "/proc/self/fd") || VG_(strcmp)(path, ppfd)) return True; > +/* Filter out Valgrind's internal file descriptors from getdents results. > + Prevents client programs from seeing or interfering with Valgrind's > + reserved FDs when scanning /proc/self/fd. */ Note that programs should be able to interfer with valgrind's reserved FDs because we (should) check that at every system call that takes or returns file descriptors. Please document orig_size, number of bytes (not number of dirents), and return value, number in bytes of total (remaining) dirents. > +static SizeT filter_valgrind_fds_from_getdents(struct vki_dirent *dirp, > SizeT orig_size) > +{ > + struct vki_dirent *dp = dirp; > + struct vki_dirent *write_pos = dirp; > + SizeT bytes_processed = 0; > + SizeT new_size = 0; > + > + while (bytes_processed < orig_size) { > + Bool keep_entry = True; > + const HChar *name = dp->d_name; > + > + if (VG_(strcmp)(name, ".") != 0 && VG_(strcmp)(name, "..") != 0) { > + Bool is_numeric = True; > + > + /* Check if filename is numeric (potential FD number) */ > + for (Int i = 0; name[i] != '\0'; i++) { > + if (name[i] < '0' || name[i] > '9') { > + is_numeric = False; > + break; > + } > + } So, this is an should not happen? Should it have an assert? Or is it OK to silently keep these entries? > + > + /* Filter out Valgrind FDs by checking against reserved FD range */ > + if (is_numeric && name[0] != '\0') { When can name[0] be '\0'? Is that a valid entry? > + Int fd = VG_(strtoll10)(name, NULL); > + /* Hide FDs beyond soft limit or Valgrind's output FDs */ > + if (fd >= VG_(fd_soft_limit) || > + fd == VG_(log_output_sink).fd || > + fd == VG_(xml_output_sink).fd) > + keep_entry = False; > + } > + } > + > + if (keep_entry) { > + if (write_pos != dp) > + VG_(memmove)(write_pos, dp, dp->d_reclen); > + new_size += dp->d_reclen; > + write_pos = (struct vki_dirent *)((HChar *)write_pos + > dp->d_reclen); > + } > + > + bytes_processed += dp->d_reclen; > + dp = (struct vki_dirent *)((HChar *)dp + dp->d_reclen); > + } > + > + return new_size; > +} OK, nice. -- You are receiving this mail because: You are watching all bug changes.
