This is an archive of the discontinued LLVM Phabricator instance.

Introduce ReservedAddressRange to sanitizer_common.
ClosedPublic

Authored by flowerhack on Oct 18 2017, 2:48 PM.

Details

Summary

Fixed version of https://reviews.llvm.org/D38437 (fixes Win/Fuchsia failures).

Creating a new revision, since the old one was getting a bit old/crowded.

In Fuchsia, MmapNoAccess/MmapFixedOrDie are implemented using a global
VMAR, which means that MmapNoAccess can only be called once. This works
for the sanitizer allocator but *not* for the Scudo allocator.

Hence, this changeset introduces a new ReservedAddressRange object to
serve as the new API for these calls. In this changeset, the object
still calls into the old Mmap implementations.

The next changeset two changesets will convert the sanitizer and scudo
allocators to use the new APIs, respectively. (ReservedAddressRange will
replace the SecondaryHeader in Scudo.)

Finally, a last changeset will update the Fuchsia implementation.

Diff Detail

Event Timeline

flowerhack created this revision.Oct 18 2017, 2:48 PM
flowerhack added reviewers: alekseyshl, cryptoad, phosek.

@phosek ; if you could run this on Windows for me i'd much appreciate it!

cryptoad added inline comments.Oct 20 2017, 4:19 PM
lib/sanitizer_common/sanitizer_win.cc
252

To avoid any further problem with Windows, it might be worth enforcing addr == base_ and size == size_ in just this one.

lib/sanitizer_common/tests/sanitizer_common_test.cc
14

I don't think this is going to work for Windows.

flowerhack marked 2 inline comments as done.
flowerhack added inline comments.
lib/sanitizer_common/tests/sanitizer_common_test.cc
14

gotcha, updated it to work with win APIs.

still don't have a test environment, alas-- phosek@, can you run it on your box? thanks!

cryptoad edited edge metadata.Oct 25 2017, 10:50 AM

I am gonna test with the current version of the patch minus the inline comment and let you know what's up (on Linux & Android).

lib/sanitizer_common/sanitizer_common.h
136

This one and the next should be void *blah() const {blah;} (as we did before, I am not sure why it came back again that way).

flowerhack marked an inline comment as done.
flowerhack added inline comments.
lib/sanitizer_common/sanitizer_common.h
136

oh weird. fixed!

So it looks good/works on Linux/Android in my local testing environment.
I'll let someone else comment further.
The only thing that I can see being somewhat off is the use of msync/FlushViewOfFile. I think those tests shouldn't rely on a platform specific function like this. What was the reason for it?

chatted w/ cryptoad@ a bit offline; decided the msync checks were unnecessary & removed them

flowerhack updated this revision to Diff 120314.
cryptoad added inline comments.Oct 27 2017, 8:37 AM
lib/sanitizer_common/tests/sanitizer_common_test.cc
353

memcpy(buffer, reinterpret_cast<void *>(res), init_size);

flowerhack marked an inline comment as done.
cryptoad added inline comments.Oct 27 2017, 9:30 AM
lib/sanitizer_common/sanitizer_fuchsia.cc
258

Same: CHECK_EQ(..., true) -> CHECK()

261

Here you actually want CHECK_LE(size, size_).
If addr_as_void == base_ then CHECK_LE(size, (base_as_uptr + size_) - addr) -> CHECK_LE(size, size_)
If addr + size == base_as_uptr + size_ then that CHECK_LE is true anyway, but with size <= size_ you ensure there is no overflow.
And remove the comment on the line before.

lib/sanitizer_common/sanitizer_posix_libcdep.cc
365

Same: CHECK_EQ(..., true) -> CHECK()

368

Same CHECK_LE(size, size_) (comment on the previous line becomes extraneous).

lib/sanitizer_common/sanitizer_win.cc
252

Use CHECK((addr_as_void == base_) && (size == size_)) rather than CHECK_EQ(..., true)

254

Unneeded for Windows since size == size_ and base_ == addr.

flowerhack marked 5 inline comments as done.
cryptoad accepted this revision.Oct 27 2017, 12:20 PM

Linux/Android tests pass in my local environment.
Might want another LGTM from someone else though.

This revision is now accepted and ready to land.Oct 27 2017, 12:20 PM

kindly ping @alekseyshl , if you wouldn't mind? :)

alekseyshl accepted this revision.Oct 30 2017, 10:46 AM
cryptoad closed this revision.Oct 30 2017, 10:56 AM