Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ extern char ***_NSGetArgv(void);
# include <libkern/OSAtomic.h>
# include <mach-o/dyld.h>
# include <mach/mach.h>
# include <mach/mach_error.h>
# include <mach/mach_time.h>
# include <mach/vm_statistics.h>
# include <malloc/malloc.h>
Expand Down Expand Up @@ -1265,17 +1266,35 @@ uptr FindAvailableMemoryRange(uptr size, uptr alignment, uptr left_padding,
kr = mach_vm_region_recurse(mach_task_self(), &address, &vmsize, &depth,
(vm_region_info_t)&vminfo, &count);

// There are cases where going beyond the processes' max vm does
// not return KERN_INVALID_ADDRESS so we check for going beyond that
// max address as well.
if (kr == KERN_INVALID_ADDRESS || address > max_vm_address) {
if (kr == KERN_SUCCESS) {
// There are cases where going beyond the processes' max vm does
// not return KERN_INVALID_ADDRESS so we check for going beyond that
// max address as well.
if (address > max_vm_address) {
address = max_vm_address;
kr = -1; // break after this iteration.
}

if (max_occupied_addr)
*max_occupied_addr = address + vmsize;
} else if (kr == KERN_INVALID_ADDRESS) {
// 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.

// We will break after this iteration anyway since kr != KERN_SUCCESS
} else if (kr == KERN_DENIED) {
Copy link
Contributor

@wrotki wrotki Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this part up, to follow the 'early return/exit/Die' principle, otherwise I find it a bit harder to read (else as applied to what condition(s)? Once KERN_DENIED handling is out of the way, the following happy path logic has less variables (i.e. there's one less } else if () {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew I saw it somewhere but only found it now, in Dan's comments to your other PR:

https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

Copy link
Contributor Author

@ndrewh ndrewh Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see what @DanBlackwell thinks, but I sort of prefer it the way it is right now since all of the branches are kr == <something> (it's basically a switch: but we can't do that here easily because of the break...). IMO it would be worse to try to move both of the early exits above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this feels a little clunky, but the point about the else here being equivalent to the default case of a switch is why I think the current layout is fine. And there are enough comments here to understand why we're setting the different variables.

Report("ERROR: Unable to find a memory range for dynamic shadow.\n");
Report("HINT: Ensure mach_vm_region_recurse is allowed under sandbox.\n");
Die();
} else {
if (max_occupied_addr) *max_occupied_addr = address + vmsize;
Report(
"WARNING: mach_vm_region_recurse returned unexpected code %d (%s)\n",
kr, mach_error_string(kr));
DCHECK(false && "mach_vm_region_recurse returned unexpected code");
break; // address is not valid unless KERN_SUCCESS, therefore we must not
// use it.
}

if (free_begin != address) {
// We found a free region [free_begin..address-1].
uptr gap_start = RoundUpTo((uptr)free_begin + left_padding, alignment);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Check that if mach_vm_region_recurse is disallowed by sandbox, we report a message saying so.

// RUN: %clangxx_asan -O0 %s -o %t
// RUN: not %run sandbox-exec -p '(version 1)(allow default)(deny syscall-mig (kernel-mig-routine mach_vm_region_recurse))' %t 2>&1 | FileCheck --check-prefix=CHECK-DENY %s
// RUN: not %run %t 2>&1 | FileCheck --check-prefix=CHECK-ALLOW %s
// RUN: %clangxx_asan -O3 %s -o %t
// RUN: not %run sandbox-exec -p '(version 1)(allow default)(deny syscall-mig (kernel-mig-routine mach_vm_region_recurse))' %t 2>&1 | FileCheck --check-prefix=CHECK-DENY %s
// RUN: not %run %t 2>&1 | FileCheck --check-prefix=CHECK-ALLOW %s

// sandbox-exec isn't available on iOS
// UNSUPPORTED: ios

// x86_64 does not use ASAN_SHADOW_OFFSET_DYNAMIC
// UNSUPPORTED: x86_64-darwin || x86_64h-darwin

#include <stdlib.h>

int main() {
char *x = (char *)malloc(10 * sizeof(char));
free(x);
return x[5];
// CHECK-ALLOW: {{.*ERROR: AddressSanitizer: heap-use-after-free on address}}
// CHECK-DENY-NOT: {{.*ERROR: AddressSanitizer: heap-use-after-free on address}}
// CHECK-ALLOW: {{READ of size 1 at 0x.* thread T0}}
// CHECK-ALLOW: {{ #0 0x.* in main}}
// CHECK-ALLOW: {{freed by thread T0 here:}}
// CHECK-ALLOW: {{ #0 0x.* in free}}
// CHECK-ALLOW: {{ #1 0x.* in main}}
// CHECK-ALLOW: {{previously allocated by thread T0 here:}}
// CHECK-ALLOW: {{ #0 0x.* in malloc}}
// CHECK-ALLOW: {{ #1 0x.* in main}}
// CHECK-DENY: {{.*HINT: Ensure mach_vm_region_recurse is allowed under sandbox}}
}