[sanitizer] Change Mmap*NoAccess to return nullptr on error

[cryptoad]

Summary

MmapNoAccess & MmapFixedNoAccess return directly the result of
internal_mmap, as opposed to other Mmap functions that return nullptr.

This inconsistency leads to some confusion for the callers, as some check for
~(uptr)0 (MAP_FAILED) for failure (while it can fail with -ENOMEM for
example).

Two potential solutions: change the callers, or make the functions return
nullptr on failure to follow the precedent set by the other functions.
The second option looked more appropriate to me.

Correct the callers that were wrongly checking for ~(uptr)0 or
MAP_FAILED.

TODO for follow up CLs:

• There are a couple of internal_mmap calls in XRay that check for MMAP_FAILED as a result as well (cc: @dberris); they should use internal_iserror;

[kristina]

I would expect it to just forward the return code from the original call since sanitizes already abstract away a lot of things, having more even more abstractions internally seems over the top as they're not public interfaces. I think it's best to fix the incorrect uses and perhaps document it in a more obvious way.

[cryptoad]

I guess after scavenging further, my initial patch is not correct, as other functions return nullptr on failure.
I am open to anything if we can get a consensus on what should be returned.

Here is the current state:

MmapOrDie: pointer or death
MmapOrDieOnFatalError: pointer, nullptr, or death
MmapAlignedOrDieOnFatalError: pointer, nullptr, or death
MmapNoReserveOrDie: pointer or death
MmapFixedImpl: pointer, nullptr, or death
MmapNoAccess: syscall return value (eg: pointer or -error) [on Windows it returns the result of VirtualAlloc which is NULL on failure)
MmapFixedNoAccess: syscall return value (eg: pointer or -error) [same as above for Windows]

The current wrong usage:
sanitizer::SizeClassAllocator64::Init: checks for failure with ~(uptr)0 (while it's the syscall return value)
sanitizer::LargeMmapAllocatorPtrDynamicArray::Init: checks for failure with nullptr (while it's the syscall return value)
scudo::LargeMmapAllocator::Allocate: checks for failure with ~(uptr)0 (while it's the syscall return value)
xray::profileCollectorService::allocateBuffer: checks for failure with MMAP_FAILED (while it's the syscall return value)
same with __xray::Allocator::Alloc & alocRaw

[cryptoad]

Other wrong use:
hwasan::MapDynamicShadow: checks for failure with ~(uptr)0 (while it's the syscall return value)
asan::FindDynamicShadowStart: same as above
__asan::PremapShadow: same as above

[alekseyshl]

I do support the unification of the return value, and if no call site is currently care for the actual syscall return value, I'd say, let's make all of them return pointer, nullptr or die. MMAP_FAILED might be a reasonable common ground too, but seems like nullptr is used in many places already and Windows returns NULL on failure, so a bit more unification across platforms here.

[cryptoad]

Updated proposal: make Mmap*NoAccess return nullptr on failure (like the
other Mmap functions).
Modify callers that were checking for ~(uptr)0 to now check for nullptr.

[kristina]

In D50940#1206182, @alekseyshl wrote:

I do support the unification of the return value, and if no call site is currently care for the actual syscall return value, I'd say, let's make all of them return pointer, nullptr or die. MMAP_FAILED might be a reasonable common ground too, but seems like nullptr is used in many places already and Windows returns NULL on failure, so a bit more unification across platforms here.

I think that's a reasonable common ground, seems much better than sanitizers being plagued with the TLV errno idiom, so something like a CHECK on a failed mmap call or just returning NULL (I think latter is better since this concerns the common code used by all sanitizes and some may want to recover).

[cryptoad]

Ping!

[yroux]

Hi Kostya,

Some AArch64 bots are broken since this patch was cheched in
(test/msan/mmap.cc checks for MAP_FAILED), logs are available here:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5703
http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/5703/steps/ninja%20check%202/logs/FAIL%3A%20MemorySanitizer-AARCH64%3A%3A%20mmap.cc

Thanks
Yvan

[cryptoad]

Link to test source: https://github.com/llvm-mirror/compiler-rt/blob/master/test/msan/mmap.cc#L78
Last test output: 0xf00000000
Link to msan mmap interceptor: https://github.com/llvm-mirror/compiler-rt/blob/master/lib/msan/msan_interceptors.cc#L939

As far as I understand, this test is failing because 0xf00000000 is not within the allowed AddrMapping for aarch64.
I am not sure how those mappings where added to the list in the first place.
Looking at mmap_interceptor for msan, I don't think the patch impacts it at all (it just calls real_mmap)

I'll setting up an aarch64 environment, I'll try and add the new mapping(s) and see how it fairs (unless someone can beat me to it - I only have a BBB for AArch64 so it's going to take a while)

lib/asan/asan_linux.cc

#endif
 
   uptr granularity = GetMmapGranularity();
   uptr alignment = granularity * 8;
   uptr left_padding = granularity;
   uptr shadow_size = RoundUpTo(kHighShadowEnd, granularity);
   uptr map_size = shadow_size + left_padding + alignment;
 
   uptr map_start = (uptr)MmapNoAccess(map_size);
-  CHECK_NE(map_start, ~(uptr)0);
+  CHECK(map_start);
 
   uptr shadow_start = RoundUpTo(map_start + left_padding, alignment);
   UnmapFromTo(map_start, shadow_start - left_padding);
   UnmapFromTo(shadow_start + shadow_size, map_start + map_size);
 
   return shadow_start;
 }
 

lib/asan/asan_premap_shadow.cc

 uptr PremapShadow() {
   uptr granularity = GetMmapGranularity();
   uptr alignment = granularity * 8;
   uptr left_padding = granularity;
   uptr shadow_size = PremapShadowSize();
   uptr map_size = shadow_size + left_padding + alignment;
 
   uptr map_start = (uptr)MmapNoAccess(map_size);
-  CHECK_NE(map_start, ~(uptr)0);
+  CHECK(map_start);
 
   uptr shadow_start = RoundUpTo(map_start + left_padding, alignment);
   uptr shadow_end = shadow_start + shadow_size;
   internal_munmap(reinterpret_cast<void *>(map_start),
                   shadow_start - left_padding - map_start);
   internal_munmap(reinterpret_cast<void *>(shadow_end),
                   map_start + map_size - shadow_end);
   return shadow_start;

lib/hwasan/hwasan_dynamic_shadow.cc

static uptr MapDynamicShadow(uptr shadow_size_bytes) {
   const uptr granularity = GetMmapGranularity();
   const uptr alignment = granularity * SHADOW_GRANULARITY;
   const uptr left_padding = granularity;
   const uptr shadow_size =
       RoundUpTo(shadow_size_bytes, granularity);
   const uptr map_size = shadow_size + left_padding + alignment;
 
   const uptr map_start = (uptr)MmapNoAccess(map_size);
-  CHECK_NE(map_start, ~(uptr)0);
+  CHECK(map_start);
 
   const uptr shadow_start = RoundUpTo(map_start + left_padding, alignment);
 
   UnmapFromTo(map_start, shadow_start - left_padding);
   UnmapFromTo(shadow_start + shadow_size, map_start + map_size);
 
   return shadow_start;
 }

lib/sanitizer_common/sanitizer_allocator_primary64.h

public:
   void Init(s32 release_to_os_interval_ms) {
     uptr TotalSpaceSize = kSpaceSize + AdditionalSize();
     if (kUsingConstantSpaceBeg) {
       CHECK_EQ(kSpaceBeg, address_range.Init(TotalSpaceSize,
                                              PrimaryAllocatorName, kSpaceBeg));
     } else {
       NonConstSpaceBeg = address_range.Init(TotalSpaceSize,
                                             PrimaryAllocatorName);
-      CHECK_NE(NonConstSpaceBeg, ~(uptr)0);
+      CHECK(NonConstSpaceBeg);
     }
     SetReleaseToOSIntervalMs(release_to_os_interval_ms);
     MapWithCallbackOrDie(SpaceEnd(), AdditionalSize());
     // Check that the RegionInfo array is aligned on the CacheLine size.
     DCHECK_EQ(SpaceEnd() % kCacheLineSize, 0);
   }
 
   s32 ReleaseToOSIntervalMs() const {

lib/sanitizer_common/sanitizer_mac_libcdep.cc

 // This file is shared between various sanitizers' runtime libraries and
 // implements OSX-specific functions.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_platform.h"
 #if SANITIZER_MAC
 #include "sanitizer_mac.h"
 
-#include <sys/mman.h>
-
 namespace __sanitizer {
 
 void RestrictMemoryToMaxAddress(uptr max_address) {
   uptr size_to_mmap = GetMaxUserVirtualAddress() + 1 - max_address;
   void *res = MmapFixedNoAccess(max_address, size_to_mmap, "high gap");
-  CHECK(res != MAP_FAILED);
+  CHECK(res);
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_MAC

lib/sanitizer_common/sanitizer_posix_libcdep.cc

void ReservedAddressRange::Unmap(uptr addr, uptr size) {
   UnmapOrDie(reinterpret_cast<void*>(addr), size);
 }
 
 void *MmapFixedNoAccess(uptr fixed_addr, uptr size, const char *name) {
   int fd = name ? GetNamedMappingFd(name, size) : -1;
   unsigned flags = MAP_PRIVATE | MAP_FIXED | MAP_NORESERVE;
   if (fd == -1) flags |= MAP_ANON;
 
-  return (void *)internal_mmap((void *)fixed_addr, size, PROT_NONE, flags, fd,
-                               0);
+  uptr p = internal_mmap((void *)fixed_addr, size, PROT_NONE, flags, fd, 0);
+  if (internal_iserror(p))
+    return nullptr;
+  return (void *)p;
 }
 
 void *MmapNoAccess(uptr size) {
   unsigned flags = MAP_PRIVATE | MAP_ANON | MAP_NORESERVE;
-  return (void *)internal_mmap(nullptr, size, PROT_NONE, flags, -1, 0);
+  uptr p = internal_mmap(nullptr, size, PROT_NONE, flags, -1, 0);
+  if (internal_iserror(p))
+    return nullptr;
+  return (void *)p;
 }
 
 // This function is defined elsewhere if we intercepted pthread_attr_getstack.
 extern "C" {
 SANITIZER_WEAK_ATTRIBUTE int
 real_pthread_attr_getstack(void *attr, void **addr, size_t *size);
 } // extern "C"
 

lib/scudo/scudo_allocator_secondary.h

if (UNLIKELY(Alignment > MinAlignment))
       ReservedSize += Alignment;
     const uptr PageSize = GetPageSizeCached();
     ReservedSize = RoundUpTo(ReservedSize, PageSize);
     // Account for 2 guard pages, one before and one after the chunk.
     ReservedSize += 2 * PageSize;
 
     ReservedAddressRange AddressRange;
     uptr ReservedBeg = AddressRange.Init(ReservedSize, SecondaryAllocatorName);
-    if (UNLIKELY(ReservedBeg == ~static_cast<uptr>(0)))
+    if (UNLIKELY(!ReservedBeg))
       return nullptr;
     // A page-aligned pointer is assumed after that, so check it now.
     DCHECK(IsAligned(ReservedBeg, PageSize));
     uptr ReservedEnd = ReservedBeg + ReservedSize;
     // The beginning of the user area for that allocation comes after the
     // initial guard page, and both headers. This is the pointer that has to
     // abide by alignment requirements.
     uptr CommittedBeg = ReservedBeg + PageSize;