SUMMARY On Debian 13, the following C program behaves differently under valgrind vs not under valgrind: #include <stdio.h> #include <unistd.h> #include <string.h> #include <stdio.h> int main() { char buf[64]; memset(buf, 0, 64); int ret = readlink("/proc/self/exe", buf, 64); printf("ret = %d, buf = %.64s\n", ret, buf); return 0; } STEPS TO REPRODUCE 1. Compile the program: `cc foo.c` 2. Run without valgrind: `./a.out` 3. Run with valgrind: `valgrind ./a.out` OBSERVED RESULT Without valgrind: `ret = 17, buf = /home/akuli/a.out` With valgrind: `ret = 17, buf = /home/akuli/a.outrind/memcheck-amd64-linux` Return value 17 is correct, but the rest of the buffer is overwritten by the string `/usr/libexec/valgrind/memcheck-amd64-linux`. EXPECTED RESULT Always `ret = 17, buf = /home/akuli/a.out` SOFTWARE/OS VERSIONS Debian 13 Valgrind 3.24.0 Valgrind 3.26.0 ADDITIONAL INFORMATION This is not specific to memcheck: adding `--tool=none` gives me `ret = 17, buf = /home/akuli/a.outrind/none-amd64-linux`. I tried with valgrind 3.24.0 from apt and 3.26.0 compiled from source (valgrind.org), same behavior. I think this used to work in some version of valgrind, because I got this problem when valgrinding after I upgraded to a newer Debian. This only happens with `/proc/self/exe`. If I use another symlink, it works fine. `strace ./a.out` gives `readlink("/proc/self/exe", "/home/akuli/a.out", 64)`. This can be worked around with `buf[ret] = '\0'` or by placing the program to a directory with a long name. This is not a compiler problem, because this happens with gcc and with LLVM.
I bisected this, the bug first appeared in commit 0690dc39644d15fc89813419ffcdf9754b098260.
The same problem exists with readlnkat using an absolute path (and probably any combination of dirfd being /, /proc, /proc/self, /proc/pid and the path being the relative remainder to /proc/pid/exe or /proc/self/exe, but that's a corner case that we don't handle at all). The man page says readlink() places the contents of the symbolic link pathname in the buffer buf, which has size bufsiz. readlink() does not append a terminating null byte to buf. It will (silently) truncate the contents (to a length of bufsiz characters), in case the buffer is too small to hold all of the contents. Since we aren't null terminating we may be within the letter of the law. I don't think that's a good enough excuse. Here is what is happening 1. PRE does its stuff 2. Valgrind does the client syscall which resolves the link to the tool exe 3. The post sees that it is a special case and does another syscall which resolves to the guest exename 4. The buffer gets overwritten without null termination. What we really need to do is to move the special case syscall to the PRE function.
Created attachment 188184 [details] initial patch Also needs doing for Linux realpathat (with testcase)
(In reply to Paul Floyd from comment #2) > The same problem exists with readlnkat using an absolute path (and probably > any combination of dirfd being /, /proc, /proc/self, /proc/pid and the path > being the relative remainder to /proc/pid/exe or /proc/self/exe, but that's > a corner case that we don't handle at all). > > The man page says > readlink() places the contents of the symbolic link pathname in > the buffer buf, > which has size bufsiz. readlink() does not append a terminating null > byte to buf. > It will (silently) truncate the contents (to a length of bufsiz > characters), in case > the buffer is too small to hold all of the contents. > > Since we aren't null terminating we may be within the letter of the law. I > don't think that's a good enough excuse. > > Here is what is happening > > 1. PRE does its stuff > 2. Valgrind does the client syscall which resolves the link to the tool exe > 3. The post sees that it is a special case and does another syscall which > resolves to the guest exename > 4. The buffer gets overwritten without null termination. > > What we really need to do is to move the special case syscall to the PRE > function. Are you sure? commit 0690dc39644d15fc89813419ffcdf9754b098260 says the special syscall was done/moved explicitly into the POST handler: commit 0690dc39644d15fc89813419ffcdf9754b098260 Author: Mark Wielaard <mark@klomp.org> Date: Sun Sep 22 23:24:34 2024 +0200 Implement /proc/self/exe readlink[at] fallback in POST handler Calling the readlink[at] syscall directly from the PRE handler defeats the FUSE_COMPATIBLE_MAY_BLOCK (SfMayBlock) flag. Add a POST handler that only explicitly calls the readlink[at] handler for the /proc/self/exe fallback (this should be fine unless /proc is also implemented as fuse in this process). Adjust readlink[at] GENX_ and LINX_ syswrap macros to GENXY and LINXY. https://bugs.kde.org/show_bug.cgi?id=493507 So moving it back into the PRE handler will reintroduce that bug.
I didn't check the commit message of 0690dc39644d15fc89813419ffcdf9754b098260 How about instead of SET_STATUS_from_SysRes( VG_(do_syscall3)(saved, (UWord)name, ARG2, ARG3)); something like Int oldlen = RET; SysRes newret = do_syscall3)(saved, (UWord)name, ARG2, ARG3); Int newlen = sr_Res(newret); if (newlen < oldlen) VG_(memset)((void*)(ARG2+newlen, 0, oldlen-newlen); (not yet tested)
Created attachment 188193 [details] Updated patch This time simply don't make a syscall if the path is a /proc/self/exe like thing. Just use VG_(resolved_executable).
commit 5c0f5e604bc3a4e8822ea59c488c123af6284afd (HEAD -> master, origin/master, origin/HEAD, bug514094) Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 13:48:50 2026 +0100 Bug 514094 - readlink("/proc/self/exe") overwrites buffer beyond its return value Used the reproducer as the basis for a test on Solaris and Linux.
The attached patch doesn't seem quite right; it overflows the buffer if the path doesn't fit. In my original C code, change `readlink("/proc/self/exe", buf, 64)` to `readlink("/proc/self/exe", buf, 5)`. I would expect to get `ret = 5, buf = /home`, but with the patch I get `ret = 17, buf = /home/akuli/a.out`. I think we want something like `SizeT res = min(VG_(strlen)(VG_(resolved_exename)), ARG3)` followed by a memcpy with size `res`.
You are right. I did think of that bit missed it out when juggling between Linux and Solaris testing. Will update in a moment along with readlinkat for Linux.
commit bd9edb8fcd0a8692d865e08fab2a573a4cde4c16 (HEAD -> master, origin/master, origin/HEAD) Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Sat Jan 3 21:17:00 2026 +0100 readlink[at] syswrap: limit copy to bufsiz when path is proc self exe
I don't think this is right, you are still doing a (In reply to Paul Floyd from comment #10) > commit bd9edb8fcd0a8692d865e08fab2a573a4cde4c16 (HEAD -> master, > origin/master, origin/HEAD) > Author: Paul Floyd <pjfloyd@wanadoo.fr> > Date: Sat Jan 3 21:17:00 2026 +0100 > > readlink[at] syswrap: limit copy to bufsiz when path is proc self exe This seems to add a testcase readlinkat_self.c which wasn't checked in.
Using the resolved_exename directly instead of going through do_syscall (readlink) with cl_exec_fd is smart. So we don't have a do_syscall invocation in the PRE handler. Nice. But this looks dangerous: + HChar* out_name = (HChar*)ARG3; + SizeT res = VG_(strlen)(VG_(resolved_exename)); + res = VG_MIN(res, ARG4); + VG_(strncpy)(out_name, VG_(resolved_exename), res); We never check ARG3 (ARG2 for readlink) is fully writable, so may crash on the strncpy. I think we need a check like: diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index 406e6960b0bb..aa4a0caf11f9 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -5096,8 +5096,12 @@ PRE(sys_readlink) HChar* out_name = (HChar*)ARG2; SizeT res = VG_(strlen)(VG_(resolved_exename)); res = VG_MIN(res, ARG3); - VG_(strncpy)(out_name, VG_(resolved_exename), res); - SET_STATUS_Success(res); + if (ML_(safe_to_deref)(out_name, res)) { + VG_(strncpy)(out_name, VG_(resolved_exename), res); + SET_STATUS_Success(res); + } else { + SET_STATUS_Failure(VKI_EFAULT); + } fuse_may_block = False; } }
I've added the missing files. I'll look at checking buf tomorrow.
I think that, finally, this is OK now.