Introduce ReservedAddressRange to sanitizer_common.

[flowerhack]

Summary

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.

Feedback welcome, can provide code pointers to the upcoming changesets
upon request, etc

[alekseyshl]

Code pointers to the future changes would be nice, actually.

compiler-rt/lib/sanitizer_common/sanitizer_common.h

86 ↗

(On Diff #117248)

In what way?

95 ↗

(On Diff #117248)

No one but Fuchsia will need it, I guess?

compiler-rt/lib/sanitizer_common/sanitizer_posix.cc

152 ↗

(On Diff #117248)

What's the point having it here? If sanitizer_posix_libcdep.cc is not included in the build, ReservedAddressRange::Init is not defined anyway.

[cryptoad]

For this first one, I think having the unmapping being implemented as well would be advised.

compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h

213 ↗

(On Diff #117248)

What will be the point of those two functions?

compiler-rt/lib/sanitizer_common/sanitizer_common.h

89 ↗

(On Diff #117248)

It could be better to have the parameters order mimic the MmapFixedNoAccess one: eg, address first.

90 ↗

(On Diff #117248)

enomem is too specific here, it might not translate to Windows platforms etc.
You might want something more like die_on_fatal_error or something shorter.

95 ↗

(On Diff #117248)

I think the name os_cookie is clear enough.
platform_specific_data or something to that extent maybe?

96 ↗

(On Diff #117248)

Missing a #pragma clang diagnostic pop
I'll let someone chime in about what is the pragma policy in sanitizer_common if any.

compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cc

486 ↗

(On Diff #117248)

Split the tests please.
ReservedAddressRange should work for both 32 & 64 bit and should be independent of the allocator (and particularly from the Primary).
The tested workflows should be in general area of init, valid map check, init invalid map, check and stuff like that.

[flowerhack]

responded to feedback, added more tests

compiler-rt/lib/sanitizer_common/sanitizer_common.h

86 ↗

(On Diff #117248)

oh, hm, it doesn't yet. the Fuchsia implementation uses it to check whether it's already been initialized, but i'll pull that out of here since it's confusing in this context

89 ↗

(On Diff #117248)

thanks for catching that; i had that elsewhere but forgot to fix it here!

90 ↗

(On Diff #117248)

On every platform this is implemented for (Windows, Fuchsia, POSIX), MmapFixedOrDieOnFatalError is actually just checking for the "out of memory" condition. Also, the function declaration in sanitizer_common.h specifically says "Behaves just like MmapFixedOrDie, but tolerates out of memory condition"--so I thought it made sense to just use a variable to indicate what it's *actually* checking for. Are there cases where you imagine this won't be true in the future that I'm not thinking of?

95 ↗

(On Diff #117248)

that's correct, at least for now (not sure who else would need it)

96 ↗

(On Diff #117248)

done.

to whoever plans to chime in on pragma policy: this pragma will be removed once all the patches are in. i can make a TODO, or remove it for now, whichever works better

compiler-rt/lib/sanitizer_common/sanitizer_posix.cc

152 ↗

(On Diff #117248)

good point. was trying to match MmapFixedOrDie closely as possible, but, it seems like that function never actually is included in anything that *doesn't* have access to libcdep. moved to libcdep

[flowerhack]

Oh, and re: the comment about code pointers being helpful: here's what the next three patches should look like:

• https://reviews.llvm.org/D38592 Adds new API to sanitizer_allocator
• https://reviews.llvm.org/D38593 Adds new API to scudo_allocator
• https://reviews.llvm.org/D38595 Updates implementation Fuchsia (main forcing function for the changes)

[mcgrathr]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

91 ↗

(On Diff #117904)

It should have a const char *name_ member and Init should set it.

94 ↗

(On Diff #117904)

Rather than the #pragma stuff, you could just leave this out and only add it along with the Fuchsia implementation.

[flowerhack]

responded to comments so far...

[flowerhack]

compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h

213 ↗

(On Diff #117248)

removed

[cryptoad]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

88 ↗

(On Diff #117934)

Thanks for changing the Map parameters order, Init should be changed too for consistency.

compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cc

282 ↗

(On Diff #117934)

Unmap is going to be tricky.
What if we unmap the middle? Then we end up with 2 mapped areas and the Reserved address range doesn't represent it properly.
For now you might want to enforce strong constraints: eg: either unmap the beginning, or the end, but not the middle.
eg addr==base_ or addr+size==base_+size_ or both (careful with overflows there).
That goes for all Unmap not just Fuchsia's.

compiler-rt/lib/sanitizer_common/sanitizer_win.cc

276 ↗

(On Diff #117934)

Windows unmap has even more requirements.
I don't think you can do partial unmapping (eg: VirtualFree requires the entire range of pages passed to Virtual alloc to be released).
So there should be more constraints.

compiler-rt/lib/sanitizer_common/tests/sanitizer_common_test.cc

340 ↗

(On Diff #117934)

Please add some unmapping tests as well.

[flowerhack]

responding to feedback

compiler-rt/lib/sanitizer_common/sanitizer_common.h

88 ↗

(On Diff #117934)

Init is almost always called without an address, though, just the size. Does it make sense to have the caller pass in a null address the majority of the time, rather than just having it be an optional later argument?

compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cc

282 ↗

(On Diff #117934)

wfm, fixed

compiler-rt/lib/sanitizer_common/sanitizer_win.cc

276 ↗

(On Diff #117934)

I see those constraints are enforced by a CHECK failure in the UnmapOrDie function that's being called. Are you saying those should be moved into this wrapper function?

[cryptoad]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

139 ↗

(On Diff #118091)

One nit here: CHECK includes UNLIKELY, so doing it that way, (eg: test outside of CHECK), you don't get the benefit of the UNLIKELY.
Ideally the tests would be wrapped within the CHECK (with a && "string" if neede).

[cryptoad]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

88 ↗

(On Diff #117934)

Ack. Nevermind then!

compiler-rt/lib/sanitizer_common/sanitizer_win.cc

276 ↗

(On Diff #117934)

You are right, let's leave it that way!

[alekseyshl]

Other than those couple comments, lgtm.

compiler-rt/lib/sanitizer_common/sanitizer_common.h

133 ↗

(On Diff #118091)

How about calling it Reserve()?

151 ↗

(On Diff #118091)

Can you move the implementation to .cc? It's not a template and it's not trivial.

[mcgrathr]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

133 ↗

(On Diff #118091)

The object represents the reservation and Init is canonical for "constructor that can return an error".
"Initialize the reservation object" makes sense.
"Reserve in the reservation object" makes less sense.

[flowerhack]

responding to comments

compiler-rt/lib/sanitizer_common/sanitizer_common.h

139 ↗

(On Diff #118091)

oh cool, TIL! updated

[flowerhack]

marking some stuff as done

[flowerhack]

apparently the file root / base was off; this is an attempt to rebate it so the patch applies cleanly

[flowerhack]

constexpr

[alekseyshl]

lib/sanitizer_common/sanitizer_common.h

137

Make base() and size() const.

[flowerhack]

fixing last feedback

lib/sanitizer_common/sanitizer_common.h

137

fixed

[flowerhack]

fix windows breakage

[flowerhack]

fix android tests

[flowerhack]

fix android build

lib/sanitizer_common/sanitizer_common.h

 uptr GetRSS();
 void NoHugePagesInRegion(uptr addr, uptr length);
 void DontDumpShadowMemory(uptr addr, uptr length);
 // Check if the built VMA size matches the runtime one.
 void CheckVMASize();
 void RunMallocHooks(const void *ptr, uptr size);
 void RunFreeHooks(const void *ptr);
 
+class ReservedAddressRange {
+ public:
+  uptr Init(uptr size, const char *name = nullptr, uptr fixed_addr = 0);
+  uptr Map(uptr fixed_addr, uptr size, bool tolerate_enomem = false);
+  void Unmap(uptr addr, uptr size);
+  void *base() { return base_; }
+  uptr size() { return size_; }
+
+ private:
+  void* base_;
+  uptr size_;
+  const char* name_;
+};
+
 typedef void (*fill_profile_f)(uptr start, uptr rss, bool file,
                                /*out*/uptr *stats, uptr stats_size);
 
 // Parse the contents of /proc/self/smaps and generate a memory profile.
 // |cb| is a tool-specific callback that fills the |stats| array containing
 // |stats_size| elements.
 void GetMemoryProfile(fill_profile_f cb, uptr *stats, uptr stats_size);
 

lib/sanitizer_common/sanitizer_fuchsia.cc

 void *MmapNoReserveOrDie(uptr size, const char *mem_type) {
   return MmapOrDie(size, mem_type);
 }
 
 void *MmapOrDieOnFatalError(uptr size, const char *mem_type) {
   return DoAnonymousMmapOrDie(size, mem_type, false, false);
 }
 
+uptr ReservedAddressRange::Init(uptr init_size, const char* name = nullptr,
+                                uptr fixed_addr = uptr(0)) {
+  base_ = MmapNoAccess(init_size);
+  size_ = size;
+  name_ = name;
+  return reinterpret_cast<uptr>(base_);
+}
+
+// Uses fixed_addr for now.
+// Will use offset instead once we've implemented this function for real.
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr map_size,
+                               bool tolerate_enomem = true) {
+  return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, size,
+                                               tolerate_enomem));
+}
+
+void ReservedAddressRange::Unmap(uptr addr, uptr size) {
+  void* addr_as_void = reinterpret_cast<void*>(addr);
+  uptr base_as_uptr = reinterpret_cast<uptr>(base_);
+  // Only unmap at the beginning or end of the range.
+  CHECK_EQ((addr_as_void == base_) || (addr + size == base_as_uptr + size_),
+           true);
+  // Detect overflows.
+  CHECK_LE(size, (base_as_uptr + size_) - addr);
+  UnmapOrDie(reinterpret_cast<void*>(addr), size);
+  if (addr_as_void == base_) {
+    base_ = reinterpret_cast<void*>(addr + size);
+  }
+  size_ = size_ - size;
+}
+
 // MmapNoAccess and MmapFixedOrDie are used only by sanitizer_allocator.
 // Instead of doing exactly what they say, we make MmapNoAccess actually
 // just allocate a VMAR to reserve the address space.  Then MmapFixedOrDie
 // uses that VMAR instead of the root.
 
 zx_handle_t allocator_vmar = ZX_HANDLE_INVALID;
 uintptr_t allocator_vmar_base;
 size_t allocator_vmar_size;

lib/sanitizer_common/sanitizer_posix_libcdep.cc

void *MmapFixedNoReserve(uptr fixed_addr, uptr size, const char *name) {
   if (internal_iserror(p, &reserrno))
     Report("ERROR: %s failed to "
            "allocate 0x%zx (%zd) bytes at address %zx (errno: %d)\n",
            SanitizerToolName, size, size, fixed_addr, reserrno);
   IncreaseTotalMmap(size);
   return (void *)p;
 }
 
+uptr ReservedAddressRange::Init(uptr size, const char *name, uptr fixed_addr) {
+  if (fixed_addr) {
+    base_ = MmapFixedNoAccess(fixed_addr, size, name);
+  } else {
+    base_ = MmapNoAccess(size);
+  }
+  size_ = size;
+  name_ = name;
+  return reinterpret_cast<uptr>(base_);
+}
+
+// Uses fixed_addr for now.
+// Will use offset instead once we've implemented this function for real.
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr size,
+                               bool tolerate_enomem) {
+  if (tolerate_enomem) {
+    return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr, size));
+  }
+  return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, size));
+}
+
+void ReservedAddressRange::Unmap(uptr addr, uptr size) {
+  void* addr_as_void = reinterpret_cast<void*>(addr);
+  uptr base_as_uptr = reinterpret_cast<uptr>(base_);
+  // Only unmap at the beginning or end of the range.
+  CHECK_EQ((addr_as_void == base_) || (addr + size == base_as_uptr + size_),
+           true);
+  // Detect overflows.
+  CHECK_LE(size, (base_as_uptr + size_) - addr);
+  UnmapOrDie(reinterpret_cast<void*>(addr), size);
+  if (addr_as_void == base_) {
+    base_ = reinterpret_cast<void*>(addr + size);
+  }
+  size_ = size_ - 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);
 }

lib/sanitizer_common/sanitizer_win.cc

if (p == 0) {
     char mem_type[30];
     internal_snprintf(mem_type, sizeof(mem_type), "memory at address 0x%zx",
                       fixed_addr);
     ReportMmapFailureAndDie(size, mem_type, "allocate", GetLastError());
   }
   return p;
 }
 
+// Uses fixed_addr for now.
+// Will use offset instead once we've implemented this function for real.
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr size,
+                               bool tolerate_enomem) {
+  if (tolerate_enomem) {
+    return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr, size));
+  }
+  return reinterpret_cast<uptr>(MmapFixedOrDie(uptr fixed_addr, uptr size));
+}
+
+void ReservedAddressRange::Unmap(uptr addr, uptr size) {
+  void* addr_as_void = reinterpret_cast<void*>(addr);
+  uptr base_as_uptr = reinterpret_cast<uptr>(base_);
+  // Only unmap at the beginning or end of the range.
+  CHECK_EQ((addr_as_void == base_) || (addr + size == base_as_uptr + size_),
+           true);
+  // Detect overflows.
+  CHECK_LE(size, (base_as_uptr + size_) - addr);
+  UnmapOrDie(reinterpret_cast<void*>(addr), size);
+  if (addr_as_void == base_) {
+    base_ = reinterpret_cast<void*>(addr + size);
+  }
+  size_ = size_ - size;
+}
+
 void *MmapFixedOrDieOnFatalError(uptr fixed_addr, uptr size) {
   void *p = VirtualAlloc((LPVOID)fixed_addr, size,
       MEM_COMMIT, PAGE_READWRITE);
   if (p == 0) {
     char mem_type[30];
     internal_snprintf(mem_type, sizeof(mem_type), "memory at address 0x%zx",
                       fixed_addr);
     return ReturnNullptrOnOOMOrDie(size, mem_type, "allocate");
   }
   return p;
 }
 
 void *MmapNoReserveOrDie(uptr size, const char *mem_type) {
   // FIXME: make this really NoReserve?
   return MmapOrDie(size, mem_type);
 }
 
+uptr ReservedAddressRange::Init(uptr size, const char *name, uptr fixed_addr) {
+  if (fixed_addr) {
+    base_ = MmapFixedNoAccess(fixed_addr, size, name);
+  } else {
+    base_ = MmapNoAccess(size);
+  }
+  size_ = size;
+  name_ = name;
+  return reinterpret_cast<uptr>(base_);
+}
+
+
 void *MmapFixedNoAccess(uptr fixed_addr, uptr size, const char *name) {
   (void)name; // unsupported
   void *res = VirtualAlloc((LPVOID)fixed_addr, size,
                            MEM_RESERVE, PAGE_NOACCESS);
   if (res == 0)
     Report("WARNING: %s failed to "
            "mprotect %p (%zd) bytes at %p (error code: %d)\n",
            SanitizerToolName, size, size, fixed_addr, GetLastError());

lib/sanitizer_common/tests/sanitizer_common_test.cc

for (uptr size = 4; size <= ARRAY_SIZE(buffer_1); size += 4) {
         EXPECT_TRUE(GetRandom(buffer_2, size, blocking));
         EXPECT_NE(internal_memcmp(buffer_1, buffer_2, size), 0);
       }
     }
   }
 }
 #endif
 
+TEST(SanitizerCommon, ReservedAddressRangeInit) {
+  uptr init_size = 0xffff;
+  ReservedAddressRange address_range;
+  uptr res = address_range.Init(init_size);
+  CHECK_NE(res, (void*)-1);
+  UnmapOrDie((void*)res, init_size);
+  // Should be able to map into the same space now.
+  ReservedAddressRange address_range2;
+  uptr res2 = address_range2.Init(init_size, nullptr, res);
+  CHECK_EQ(res, res2);
+
+  // TODO(flowerhack): Once this is switched to the "real" implementation
+  // (rather than passing through to MmapNoAccess*), enforce and test "no
+  // double initializations allowed"
+}
+
+TEST(SanitizerCommon, ReservedAddressRangeMap) {
+  constexpr uptr init_size = 0xffff;
+  ReservedAddressRange address_range;
+  uptr res = address_range.Init(init_size);
+  CHECK_NE(res, (void*) -1);
+
+  // Valid mappings should succeed.
+  CHECK_EQ(res, address_range.Map(res, init_size));
+
+  // Valid mappings should be readable.
+  unsigned char buffer[init_size];
+  memcpy(buffer, &res, sizeof(buffer));
+
+  // Invalid mappings should fail.
+  EXPECT_DEATH(address_range.Map(res, 0), ".*");
+
+  // TODO(flowerhack): Once this is switched to the "real" implementation, make
+  // sure you can only mmap into offsets in the Init range.
+}
+
+TEST(SanitizerCommon, ReservedAddressRangeUnmap) {
+  uptr PageSize = GetPageSizeCached();
+  uptr init_size = PageSize * 4;
+  ReservedAddressRange address_range;
+  uptr base_addr = address_range.Init(init_size);
+  CHECK_NE(base_addr, (void*)-1);
+  CHECK_EQ(base_addr, address_range.Map(base_addr, init_size));
+
+  // Unmapping at the beginning should succeed.
+  address_range.Unmap(base_addr, PageSize);
+  CHECK_EQ(base_addr + PageSize, address_range.base());
+  CHECK_EQ(init_size - PageSize, address_range.size());
+
+  // Unmapping at the end should succeed.
+  uptr old_size = address_range.size();
+  void* old_base = address_range.base();
+  uptr new_start = reinterpret_cast<uptr>(address_range.base()) +
+                   address_range.size() - PageSize;
+  address_range.Unmap(new_start, PageSize);
+  CHECK_EQ(old_size - PageSize, address_range.size());
+  CHECK_EQ(old_base, address_range.base());
+
+  // Unmapping in the middle of the ReservedAddressRange should fail.
+  EXPECT_DEATH(address_range.Unmap(base_addr + 0xf, 0xff), ".*");
+}
+
 }  // namespace __sanitizer