Page MenuHomePhabricator

[asan] Be more careful and verbose when allocating dynamic shadow memory
ClosedPublic

Authored by kubamracek on Feb 14 2018, 2:34 PM.

Details

Summary

FindAvailableMemoryRange can currently overwrite existing memory (by restricting the VM below addresses that are already used). This patch adds a check to make sure we don't restrict the VM space too much. We are also now more explicit about why the lookup failed and print out verbose values.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek created this revision.Feb 14 2018, 2:34 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptFeb 14 2018, 2:34 PM
lib/asan/asan_mac.cc
66 ↗(On Diff #134316)

yay logging yay!

75 ↗(On Diff #134316)

I don't quite understand semantics here.
max_occupied_vm to me means "maximal size of occupied virtual memory".
Then why it's a problem if a new maximal size is smaller? Shouldn't it be a problem if it's larger?

lib/sanitizer_common/sanitizer_mac.cc
901 ↗(On Diff #134316)

Is this assignment necessary? Maximum size is zero unless set otherwise?

kubamracek added inline comments.Feb 23 2018, 5:30 PM
lib/asan/asan_mac.cc
75 ↗(On Diff #134316)

max_occupied_vm is the largest address that it already occupied by something. We're trying to restrict the VM space with new_max_vm being the new VM limit. This check is to make sure we don't restrict too much (which would overwrite memory that is already used).

Suggestions for better name of max_occupied_vm?

lib/sanitizer_common/sanitizer_mac.cc
901 ↗(On Diff #134316)

I'd rather not leave garbage in max_vm_address if for some reason the loop below doesn't find any regions at all.

kubamracek added inline comments.Feb 23 2018, 5:31 PM
lib/sanitizer_common/sanitizer_mac.cc
901 ↗(On Diff #134316)

I mean, I don't want the semantics of this (or any) function to only partially (on some paths) initialize pointer-passed values...

lib/asan/asan_mac.cc
75 ↗(On Diff #134316)

largest_occupied_vm? Almost the same, but at least to me does quite have same connotation, but that's a not important nit.. In any case, maybe this should be explained in a doxygen comment for FindAvailableMemoryRange?

fjricci added inline comments.Feb 24 2018, 8:39 AM
lib/asan/asan_mac.cc
75 ↗(On Diff #134316)

max_occupied_addr or max_occupied_vm_addr, perhaps?

Renamed variable.

This revision is now accepted and ready to land.Feb 26 2018, 10:13 AM
This revision was automatically updated to reflect the committed changes.