Created attachment 169667 [details] patch to accurately report leaks for fds 0, 1, 2 SUMMARY If a program closes any std fd then reopens it as non-std, Memcheck's --track-fds=yes report is inaccurate. If fds 0, 1, or 2 are in use, --track-fds=yes and --track-fds=all treat them as std (regardless of whether they are or not), leading to erroneous output. Leak messages are consequently omitted with --track-fds=yes. STEPS TO REPRODUCE Run a program that closes one of the std fds, and reopens another file in its place without closing it: // test.c #include <assert.h> #include <stdio.h> #include <unistd.h> int main(void) { FILE *fp; assert(fclose(stdout) == 0); fp = fopen("newfile.txt", "w"); assert(fileno(fp) == STDOUT_FILENO); return 0; } OBSERVED RESULT $ gcc -g -Wall test.c -o test && valgrind --leak-check=yes --track-fds=yes ./test ==422217== Memcheck, a memory error detector ==422217== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al. ==422217== Using Valgrind-3.24.0.GIT and LibVEX; rerun with -h for copyright info ==422217== Command: ./test ==422217== ==422217== ==422217== FILE DESCRIPTORS: 3 open (3 std) at exit. ==422217== ==422217== HEAP SUMMARY: ==422217== in use at exit: 472 bytes in 1 blocks ==422217== total heap usage: 1 allocs, 0 frees, 472 bytes allocated ==422217== ==422217== LEAK SUMMARY: ==422217== definitely lost: 0 bytes in 0 blocks ==422217== indirectly lost: 0 bytes in 0 blocks ==422217== possibly lost: 0 bytes in 0 blocks ==422217== still reachable: 472 bytes in 1 blocks ==422217== suppressed: 0 bytes in 0 blocks ==422217== Reachable blocks (those to which a pointer was found) are not shown. ==422217== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==422217== ==422217== For lists of detected and suppressed errors, rerun with: -s ==422217== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Note that Memcheck reports 3 std fds open at exit, does not report a leak message, and does not report any errors. EXPECTED RESULT Memcheck should report 2 std fds open at exit, a leak message should be outputted for the newly opened fd, and the error summary should list 1 error.
Nice. Thanks for catching this. Yeah, it makes sense to start "tracking" std fds once they are closed (and reopened). One thought I had was that maybe this shouldn't be special cased for just stdin/out/err (0,1,2). It seems to me that those are just a special case of inherited file descriptors. Currently out testcases themselves "cheat". We have some special CLOSE_INHERITED_FDS macro in there to shut valgrind --track-fds up about any file descriptors the tests might inherit (some versions of perl "leak" them in out testsuite). We could track those, together with 0, 1, 2 through VG_(init_preopened_fds) at program startup.
Treating the std fds as "generic" inherited fds would also handle the case where a program is started with stdin/out/err already closed.
For each inherited fd VG_(init_preopened_fds) calls ML_(record_fd_open_named)(-1, fd); So the ThreadId will be -1. Which ML_(record_fd_open_with_given_name) documents as "If the tid is -1, this indicates an inherited fd." Which in practice means the struct OpenFd where field will be set to NULL ExeContext *where; /* NULL if inherited from parent */ So what we could probably do, instead of having a special static Bool std_fds[3], is checking for where == NULL instead. Which would handle all inherited file descriptors, not just 0, 1, 2. Does that make sense? Would that be useful?
That makes sense. I think you're right in that there doesn't seem to be a good reason to isolate the "std" fds, as those are treated differently only by convention. Like you mentioned, my proposed fix is not really solving the issue. There's just not a great way of reliably determining if a std fd is open or not, so I don't love the --track-fds=yes option as it stands. If we're implementing your suggestion, there are certain potentially useful cases that this option would now be ignoring, like inheriting a pipe, where one might want to know if the child has closed its end of the pipe (as is good practice). I think this is fine as long as the "all" option exists. The user can make an appropriate decision about what they'd want to track. I implemented your change along with a minor documentation change for the --track-fds=yes/all options in m_main.c, will attach.
Created attachment 170507 [details] patch to modify track-fds option
Thanks, sorry for the long delay getting this in. I updated NEWS and tweaked the expected test result files to match the new output. commit 9f0e4107c140b47ea2a9c097afcac73a8454e17f Author: Mark Wielaard <mark@klomp.org> Date: Thu Jan 23 15:50:52 2025 +0100 Treat all inherited file descriptors the same with --track-fds. We used to special case 0, 1, 2 (stdin/out/err) specially even when they were not inherited (anymore). Now the --track-fds=[yes|all] option treats all inherited file descriptors the same. And if any inherited file descriptor gets closed and reopened then they are now treated as normal non-inherited file descriptors. https://bugs.kde.org/show_bug.cgi?id=487296