[sanitizer][fuchsia] Fix VMAR leak

[cryptoad]

Summary

Destroy and close a range's vmar if all its memory was unmapped.

This addresses some performance regression due to the proliferation of vmars
when Secondary backed allocations are concerned with Scudo on Fuchsia.

When a Secondary backed allocation was freed, the associated
ReservedAddressRange was going away after unmapping the entirety of the
mapping, but without getting rid of the associated vmar properly (which
was created specifically for that mapping). This resulted in an increase of
defunct vmars, that in turn slowed down further new vmar allocations.

This appears to solve ZX-2560/ZX-2642, at least on QEMU.

[cryptoad]

Some before & after numbers for one of the benchmarks involved:

1552377     207294    1121659    2214385    1550481 nanoseconds              N/A Thread/CreateAndJoin
451883      35547     374475     984192     448331 nanoseconds              N/A Thread/CreateAndJoin

[mseaborn]

lib/sanitizer_common/sanitizer_fuchsia.cc

293

Note that if you're destroying the VMAR, you don't need to unmap any mappings from it first, because destroying the VMAR will remove those mappings. So you can avoid a syscall in that case.

[cryptoad]

Skip the UnmapOrDieVmar call when unmapping the whole mapping, as
vmar_destroy will take care of this. We still have to do some bookkeeping
via DecreaseTotalMmap.

[mcgrathr]

lib/sanitizer_common/sanitizer_fuchsia.cc

283

This comment is a little ambiguous or misleading.
Destroying the VMAR is what unmaps the whole range.
Closing the VMAR handle just releases a now-useless resource; it has no effect itself on the state of mappings.

290

This seems questionable and at least needs comments.
By moving base_ up you are telling the local bookkeeping that the reservation starts at the new base.
But the actual VMAR still exists and will always cover the full range set at its creation.
So the address space between the old base_ and the new base_ is still reserved, but you've lost track of it.

[cryptoad]

Correct a comment to reflect that it is the destruction of the vmar that is
responsible for the unmapping.

[cryptoad]

lib/sanitizer_common/sanitizer_fuchsia.cc

290

I agree with you, the ReservedAddressRange in their current state do not allow distinction between committed & reserved.
I think the original idea was to allow multiple Unmap to occur at the extremities of the mapping, but the use is currently confined to a full unmapping or a single pruning on either side in the event of Secondary aligned allocation.
I can skip the update of base_ & size_ to keep track of the reserved area if it feels more accurate to you, it shouldn't impact the current usage scenario.

[mcgrathr]

lib/sanitizer_common/sanitizer_fuchsia.cc

290

That does seem preferable, and with some comments about reserved regions that won't be ever be reused if that's the case.

[cryptoad]

Do not update base_ & size_ to reflect the fact that the reserved range
remains unchanged. Adding a comment to clarify that partial unmapping still
leaves the memory reserved.

[mcgrathr]

lgtm

[mseaborn]

Suggestion: you could start the commit message with:
"[sanitizer][fuchsia] Fix VMAR leak

Destroy and close a range's vmar if all its memory was unmapped.
..."

i.e. Start the commit message with the goal (fix the leak) and secondarily say how that is achieved. This should make it more obvious to the reader what the change does. Otherwise it's not so obvious that this fixes a leak.

I'm not very familiar with this code (and I'm not sure whether this fixes the leak in all cases) so I'll leave the rest of the review to Roland.

lib/sanitizer_common/sanitizer_fuchsia.cc

if (status != ZX_OK) {
     CHECK("unable to unmap" && 0);
   }
 
   DecreaseTotalMmap(size);
 }
 
 void ReservedAddressRange::Unmap(uptr addr, uptr size) {
   CHECK_LE(size, size_);
-  if (addr == reinterpret_cast<uptr>(base_))
-    // If we unmap the whole range, just null out the base.
-    base_ = (size == size_) ? nullptr : reinterpret_cast<void*>(addr + size);
-  else
+  const zx_handle_t vmar = static_cast<zx_handle_t>(os_handle_);
+  if (addr == reinterpret_cast<uptr>(base_)) {
+    if (size == size_) {
+      // Destroying the vmar effectively unmaps the whole mapping.
+      _zx_vmar_destroy(vmar);
+      _zx_handle_close(vmar);
+      os_handle_ = static_cast<uptr>(ZX_HANDLE_INVALID);
+      DecreaseTotalMmap(size);
+      return;
+    }
+  } else {
     CHECK_EQ(addr + size, reinterpret_cast<uptr>(base_) + size_);
-  size_ -= size;
-  UnmapOrDieVmar(reinterpret_cast<void *>(addr), size,
-                 static_cast<zx_handle_t>(os_handle_));
+  }
+  // Partial unmapping does not affect the fact that the initial range is still
+  // reserved, and the resulting unmapped memory can't be reused.
+  UnmapOrDieVmar(reinterpret_cast<void *>(addr), size, vmar);
 }
 
 // This should never be called.
 void *MmapFixedNoAccess(uptr fixed_addr, uptr size, const char *name) {
   UNIMPLEMENTED();
 }
 
 void *MmapAlignedOrDieOnFatalError(uptr size, uptr alignment,