This is an archive of the discontinued LLVM Phabricator instance.

Split Mprotect into MmapNoAccess and MprotectNoAccess to be more portable
ClosedPublic

Authored by timurrrr on Apr 10 2015, 7:55 AM.

Details

Reviewers
glider
Summary

On Windows, we have to know if a memory to be protected is mapped or not.
On POSIX, Mprotect was semantically different from mprotect most people know.

Diff Detail

Event Timeline

timurrrr updated this revision to Diff 23597.Apr 10 2015, 7:55 AM
timurrrr retitled this revision from to Split Mprotect into MmapNoAccess and MprotectNoAccess to be more portable.
timurrrr updated this object.
timurrrr edited the test plan for this revision. (Show Details)
timurrrr added a reviewer: glider.
timurrrr added a subscriber: Unknown Object (MLST).

I'll go ahead and commit it, since Alexander is out of the office already and we've previously discussed the new naming.
Fingers crossed, this doesn't break on Mac.

glider accepted this revision.Apr 10 2015, 8:26 AM
glider edited edge metadata.

LGTM with nits.

lib/asan/asan_rtl.cc
300

The argument name "a" is meaningless, can you please fix?
(I know it was meaningless before)

lib/sanitizer_common/sanitizer_posix.cc
202

Why not return an uptr here?
IIRC the result of internal_mmap() is an uptr, which is cast to void* here, returned and then cast to an uptr by the callers of MmapNoAccess().

This revision is now accepted and ready to land.Apr 10 2015, 8:26 AM
timurrrr closed this revision.Apr 10 2015, 8:36 AM
timurrrr added inline comments.
lib/asan/asan_rtl.cc
300

Good point, done [r234604]

lib/sanitizer_common/sanitizer_posix.cc
202

We've discussed this offline and decided not to do that for consistency with other Mmaps.
If anything, we should change the types of the parameters...

hjl.tools added inline comments.
lib/sanitizer_common/sanitizer_linux.cc
132

This is incorrect on Linux/x86-64. Pointers passed to
internal_syscall should be casted to uptr first. Otherwise,
they won't be properly extended to 64-bit for x32. Need
something like

return internal_syscall(SYSCALL(mprotect), (uptr)addr, length, prot);

timurrrr added inline comments.Apr 13 2015, 5:19 AM
lib/sanitizer_common/sanitizer_linux.cc
113

I suggest we put a comment here describing that we must cast any pointer argument to uptr to be compatible with X32.

132

Thanks, done as r234748.