Bug 210268

Summary: darwin merge broke mmap2/mprotect behavior on linux
Product: [Developer tools] valgrind Reporter: Dan Kegel <dank>
Component: generalAssignee: Julian Seward <jseward>
Status: RESOLVED DUPLICATE    
Severity: normal CC: jakub, njn
Priority: NOR    
Version: unspecified   
Target Milestone: blocking3.5.1   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:

Description Dan Kegel 2009-10-12 02:38:39 UTC
Version:            (using Devel)
OS:                Linux
Installed from:    Compiled sources

On x86 linux (ubuntu 9.04), the test program

#include <sys/mman.h>
#include <string.h>
#include <assert.h>
int main(int argc, char **argv)
{
    char *buf = (char *)0x7f000000;
    char *p = mmap(buf, 1114112, PROT_NONE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0);
    int err;
    assert(p == buf);
    err = mprotect(buf, 65536, PROT_READ|PROT_WRITE);
    assert(err == 0);
    strcpy(p, "hello, mmap");
}

runs fine in valgrind right up to 
"r10156 | njn | 2009-05-27 18:53:07 -0700 (Wed, 27 May 2009) | 7 lines
Merge the DARWIN branch onto the trunk."

Afterwards, valgrind complains

Invalid write of size 1
 at memcpy (mc_replace_strmem.c:482)
 by main (foo.c:13)
 Address 0x7f00000b is not stack'd, malloc'd or (recently) free'd

It's not surprising that the darwin merge broke
something.  It *is* somewhat surprising that nobody
noticed until now.  Maybe that sequence is rare...

( Previously filed as 
http://bugs.winehq.org/show_bug.cgi?id=20303, but turned
out to be a problem in valgrind, not wine.  Fortunately,
strace revealed the test program fairly directly. )
Comment 1 Jakub Jelinek 2009-10-12 17:12:18 UTC
The problem is in the darwin memcheck/mc_main.c changes, in particular mc_new_mem_startup/mc_new_mem_mmap now doing MC_(make_mem_noaccess)(a, len);
for PROT_NONE mappings instead of MC_(make_mem_defined)(a, len);
ATM memcheck ignores change_mem_mprotect, so if something is mapped as PROT_NONE initially, it is tracked as noaccess even if it is latter on changed with mprotect.

Either this change should be reverted (or guarded with darwin define only, or limited to Addr 0 only), or mc_main.c needs to be taught to track also mprotect.  Given the comment why it doesn't track mprotect I think making it noaccess on PROT_NONE mprotect and defined on other mprotect is not desirable, so perhaps just making it defined on != PROT_NONE mprotect or walking the range and changing any noaccess ranges in it to defined.
Comment 2 Nicholas Nethercote 2009-10-12 23:32:12 UTC
Thanks for the diagnosis, Jakub.
Comment 3 Dan Kegel 2009-10-18 05:37:11 UTC
Looks like John Reiser got there first; see
bug 205541.  He even submitted a possible patch.
Comment 4 Dan Kegel 2009-10-18 05:37:29 UTC

*** This bug has been marked as a duplicate of bug 205541 ***