Created attachment 170137 [details] Patch fixing the aforementioned bug. SUMMARY On commit `faa8c5274`, in `coregrind/m_initimg/initimg-linux.c`, in `setup_client_env`, line 146/147 read: Int preload_core_path_len = vglib_len + sizeof(preload_core) + sizeof(VG_PLATFORM) + 16; For context, the definition of `preload_core` is line 128: const HChar* preload_core = "vgpreload_core"; The `sizeof(preload_core)` on line 146 evaluates the size of a pointer rather than the string behind it (unlike for `VG_PLATFORM` which is a macro). Lines 146/147 should be: Int preload_core_path_len = vglib_len + VG_(strlen)(preload_core) + sizeof(VG_PLATFORM) + 16; This code currently works thanks to the two `+ 16` which make up for the mismatch between the size of the pointer and the length of the name. Patch is attached. STEPS TO REPRODUCE N/A OBSERVED RESULT N/A EXPECTED RESULT N/A SOFTWARE/OS VERSIONS Linux: Linux 6.9.3-arch1-1 #1 SMP PREEMPT_DYNAMIC Fri, 31 May 2024 15:14:45 +0000 x86_64 GNU/Linux ADDITIONAL INFORMATION I have attempted submitting the patch on Gitlab but failed to find the repository.
Nice catch! How did you spot this if I may ask? There isn't actually a write-beyond-allocated-memory here because of the +16 magic constants added to preload_...._path_len. But should be fixed nevertheless.
I am trying to take a deep look at how valgrind works. [The Design and Implementation of Valgrind](https://valgrind.org/docs/manual/mc-tech-docs.html) is pretty old and the architecture of the project has changed a lot since. I've downloaded the source and explored, taking notes as I read. This particular line of code triggered a warning from my LSP, so it didn't prove to be much of a challenge to spot it ;)
Don’t remember seeing this in static analysis. I’ll check the other platforms.
FreeBSD and Darwin also need changes. I'll close this when I've done them. commit ecd6d353a5ec2fd12665947235186ab014b629e8 (HEAD -> master, origin/master, origin/HEAD) Author: Ethiraric <ethiraric@gmail.com> Date: Tue Jun 4 18:37:48 2024 +0200 coregrind: fix allocation length The `sizeof(preload_core)` here evaluates the size of a pointer rather than the string behind it (unlike for `VG_PLATFORM` which is a macro).
Done for FreeBSD and Darwin as well. The Solaris code is a bit different, and looks correct. Thanks for the patch!