[Sanitizers][Apple] Fix logic bugs that break RestrictMemoryToMaxAddress

[thetruestblue]

There are two logic bugs breaking RestrictMemoryToMaxAddress.

1. adding left_padding within MapDynamicShadow.

• RoundUpTo((uptr)free_begin + left_padding, alignment) already adjusts for left padding. Adding this additionally within MapDynamicShadow causes us to allocate a page larger than necessary.
• This incorrect calculation also means RestrictMemoryToMaxAddress will never find a big enough gap.

2. There is also an issue with the expectation of hitting KERN_INVALID_ADDRESS when we are beyond the addressable regions.

• For most embedded scenarios, we exceed vm_max_address without getting KREN_INVALID_ADDRESS so we setting max_occupied_address to a memory region the process doesn't have access to, beyond the max address, and that space is never marked as available so we never find a valid gap in those regions.
• At some point previous it seems the assumption was once we were beyond the Max address we could expect KREN_INVALID_ADDRESS, which is no longer true up through the extended space not given to most processes.
• Because of this, the check if (new_max_vm < max_occupied_addr) will always fail and we will never restrict the address on smaller devices.
• Additionally because of the extra page added by adding left_padding, and how we only minimally restrict the vm, there's a chance we restrict the vm only enough for the correctly calculated size of shadow. In these cases, restricting the vm max address and will always fail due to the extra page added to space size.

credit to @delcypher for the left_padding diagnosis, remembered his old radar and PR when investigating this. https://reviews.llvm.org/D85389

rdar://66603866

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (thetruestblue)

Changes

There are two logic bugs breaking RestrictMemoryToMaxAddress -- adding left_padding within MapDynamicShadow.

There is also an issue with the expectation of hitting KERN_INVALID_ADDRESS when we are beyond the addressable regions.

For most embedded scenarios, we exceed vm_max_address and setting max_occupied address to a memory region the process doesn't have access to.

Because of this, our check if (new_max_vm < max_occupied_addr) { will always fail and we will never restrict the address on smaller devices.

rdar://66603866

---

Full diff: https://github.com/llvm/llvm-project/pull/124712.diff

1 Files Affected:

• (modified) compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp (+4-3)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
index d15f30c61b5863..ade2ed2da6e4aa 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
@@ -1203,7 +1203,7 @@ uptr MapDynamicShadow(uptr shadow_size_bytes, uptr shadow_scale,
   const uptr left_padding =
       Max<uptr>(granularity, 1ULL << min_shadow_base_alignment);
 
-  uptr space_size = shadow_size_bytes + left_padding;
+  uptr space_size = shadow_size_bytes;
 
   uptr largest_gap_found = 0;
   uptr max_occupied_addr = 0;
@@ -1229,7 +1229,7 @@ uptr MapDynamicShadow(uptr shadow_size_bytes, uptr shadow_scale,
     }
     RestrictMemoryToMaxAddress(new_max_vm);
     high_mem_end = new_max_vm - 1;
-    space_size = (high_mem_end >> shadow_scale) + left_padding;
+    space_size = (high_mem_end >> shadow_scale);
     VReport(2, "FindDynamicShadowStart, space_size = %p\n", (void *)space_size);
     shadow_start = FindAvailableMemoryRange(space_size, alignment, granularity,
                                             nullptr, nullptr);
@@ -1272,10 +1272,11 @@ uptr FindAvailableMemoryRange(uptr size, uptr alignment, uptr left_padding,
     mach_msg_type_number_t count = kRegionInfoSize;
     kr = mach_vm_region_recurse(mach_task_self(), &address, &vmsize, &depth,
                                 (vm_region_info_t)&vminfo, &count);
-    if (kr == KERN_INVALID_ADDRESS) {
+    if (kr == KERN_INVALID_ADDRESS || address > GetMaxVirtualAddress()) {
       // No more regions beyond "address", consider the gap at the end of VM.
       address = max_vm_address;
       vmsize = 0;
+      kr = -1; // break after this iteration.
     } else {
       if (max_occupied_addr) *max_occupied_addr = address + vmsize;
     }

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[vitalybuka]

I abstain, and let folks more experienced in OSX to review.

[yln]

Currently, if mach_vm_region_recurse() ever returns an error different from KERN_INVALID_ADDRESS (I don't think it currently does except if the arguments are wrong, then it returns KERN_INVALID_ARGUMENT), then we would still treat the result "successful enough" to update max_occupied_addr. Is that the intention or should it be kr != KERN_SUCCESS?

[wrotki]

+1 to changing the condition to kr != KERN_SUCCESS .

[thetruestblue]

I dont know if this is the case. For this condition we simply finishing checking that iteration. A different return code would probably be more properly handled by not doing that and potentially throwing an error. I also don't know if we would expect different kernel codes in other cases (entitlements?) I would rather do a follow up PR with this change to keep this change more limited.

[delcypher]

Seems reasonable. I have a few nits though.

[delcypher]

@thetruestblue I think it would be good to also explain a little in the commit message how this ended up working. The current description makes it sound like this never worked which I'm pretty sure isn't the case.

[thetruestblue]

@thetruestblue I think it would be good to also explain a little in the commit message how this ended up working. The current description makes it sound like this never worked which I'm pretty sure isn't the case.

I only have a hypothesis for why this worked in the past, but don't have the historical experience to say I ever observed it working and I believe its been broken for some time. This code has been changed many times. I have a theory that the expectation that any space beyond max address used to return invalid, but now that value is not returned unless we surpass the expanded vm space available to some processes. I will add that to the commit message. But you are right, I do believe this worked in the past and should make that clear.