Valgrind r6703, vex r1749; same bug with the distro build of 3.2.0. Consider the following testcase: #include <stdlib.h> int main() { int a; malloc(a); } Which compiled to (doesn't seem to be anything weird): <main>: lea 0x4(%esp),%ecx and $0xfffffff0,%esp pushl 0xfffffffc(%ecx) push %ebp mov %esp,%ebp push %ecx sub $0x14,%esp mov 0xfffffff8(%ebp),%eax mov %eax,(%esp) call 8048280 <malloc@plt> add $0x14,%esp pop %ecx pop %ebp lea 0xfffffffc(%ecx),%esp ret ... No warnings are produced by memcheck. (Noticed since I need to lookup the shadow value for the malloc parameter for the tool I am hacking on, and went to check for how memcheck does it, not because of an actual bug in a program).
Oh, that's an interesting one. What happens is that Memcheck replaces malloc with its own version. This version isn't instrumented by Memcheck, so Memcheck cannot detect any dangerous use of the undefined value (eg. using it in a conditional branch) within the replacement malloc. The right thing to do would be to check definedness of the arguments at the function boundary, which is what Memcheck does for system calls (which also cannot be instrumented). It's tricky for two reasons: you don't know the sizes of the arguments, nor the locations (eg. on the stack? in registers? it may depend on how the code was compiled). In contrast, this is not a problem for system calls because you have a pre-defined set of them and the arguments are always in predefined places. The right thing to do is change the macros with which these uninstrumented calls are done, forcing them to provide the size and address of each argument, and then add some new events that let tools know about the arguments in this case. That will be a bit intrusive and backward-incompatible, but it should be done for full correctness. Thanks for pointing this out -- over five years of development on Valgrind, and you're the first person to notice this!
Similarly, uninitialized parameters to VALGRIND_MAKE_MEM_NOACCESS() VALGRIND_MAKE_MEM_UNDEFINED() VALGRIND_MAKE_MEM_DEFINED() should be reported. See https://android-review.googlesource.com/#/c/307010/14..15/runtime/base/arena_allocator.cc where it took a long time (and some luck) to track down the uninitialized value being passed to VALGRIND_MAKE_MEM_NOACCESS().
(In reply to Vladimir Marko from comment #2) Thanks for reporting the problem. Note that in revision r13361, the bug of false negative for undefined malloc args was fixed, and this bug should have been related to the commit, and closed. I guess it should be better to file a new bug related to the checking of the valgrind client requests arguments. > Similarly, uninitialized parar13361meters to > > VALGRIND_MAKE_MEM_NOACCESS() > VALGRIND_MAKE_MEM_UNDEFINED() > VALGRIND_MAKE_MEM_DEFINED() > > should be reported. See > > > https://android-review.googlesource.com/#/c/307010/14..15/runtime/base/ > arena_allocator.cc > > where it took a long time (and some luck) to track down the uninitialized > value being passed to VALGRIND_MAKE_MEM_NOACCESS().
(In reply to Philippe Waroquiers from comment #3) > (In reply to Vladimir Marko from comment #2) > Thanks for reporting the problem. > Note that in revision r13361, the bug of false negative for undefined malloc > args was fixed, and this bug should have been related to the commit, and > closed. > I guess it should be better to file a new bug related to the checking > of the valgrind client requests arguments. > > > Similarly, uninitialized parar13361meters to > > > > VALGRIND_MAKE_MEM_NOACCESS() > > VALGRIND_MAKE_MEM_UNDEFINED() > > VALGRIND_MAKE_MEM_DEFINED() > > > > should be reported. See > > > > > > https://android-review.googlesource.com/#/c/307010/14..15/runtime/base/ > > arena_allocator.cc > > > > where it took a long time (and some luck) to track down the uninitialized > > value being passed to VALGRIND_MAKE_MEM_NOACCESS(). Forked new bug https://bugs.kde.org/show_bug.cgi?id=377463 . Feel free to relate this bug to the fixing commit and close.
This was fixed by r13361 2013-04-04, but at that time, this bug was not found.