[scudo] Ensure pointer is not null

[phosek]

compiler-rt/lib/scudo/standalone/fuchsia.cpp

60

It seems like Data == nullptr implies zx_vmar_root_self so I wonder if we should instead use 0 as the base in that case. In other words:

const zx_vaddr_t VmarBase = Data ? Data->VmarBase : 0;

[abrachet]

compiler-rt/lib/scudo/standalone/fuchsia.cpp

60

That what I thought to do at first too, but I wasn't sure if that actually true that 0 was the base of the root vmar. Does the root vmar cover the entire virtual address space or just the executable's segments, I read the latter from https://fuchsia.dev/fuchsia-src/concepts/process/program_loading?hl=en#an_elf_et_dyn_file_with_no_pt_interp

@mcgrathr will know

[mcgrathr]

compiler-rt/lib/scudo/standalone/fuchsia.cpp

60

The way map is used for different things is pretty Byzantine, so it's a little hard to follow the intent here.
AFAICT the Data=nullptr (defaulted argument) cases are only used for the separate bookkeeping allocations that are not part of the main allocation arena. All of those cases have Addr=nullptr, i.e. allocate anywhere. So Data==nullptr code paths should never be relevant when Addr!=nullptr.

The only case where Addr!=nullptr is used or meant to be supported here is when it's making usable pages within the large address range it previously set up in the MAP_NOACCESS path that calls allocateVmar. This can only work if it has Data!=nullptr so knows what VMO it's working with and the base of the region created previously with MAP_NOACCESS within which Addr must lie. So the question of the equivalent to VmarBase when using the root VMAR is moot, since you must never be in this code path if you aren't using a specific previously-allocated VMAR (which you need Data to point you to).

But to answer the question for general knowledge: the root VMAR tells you the range of usable addresses for user mappings. This does not necessarily (and in fact, does not) include 0. The usable range starts at 1M or 16M or something I don't recall exactly, but the point is that there's no fixed guarantee except that you can expect "enough" user address space to be available for normal uses. It's up to the kernel what the low and high bounds are, and might vary by hardware or implementation details that aren't part of the ABI contract. So you're obliged to do the zx_info_vmar_t query to find out if you care. Separately, in the long run we should not presume that zx_vmar_root_self() returns the process's actual root VMAR (or indeed if a function by that name will be available forever) nor that the VMAR to use for unconstrained allocation is necessarily that same as what zx_vmar_root_self() returns (though obviously if that changes, this code will need to be changed to use the new source of VMAR handle for that purpose). Regardless, it's moot here since in no event would you want to compute the absolute base of the whole address range potentially available for any kind of allocation since nothing in an allocator has any business picking fixed addresses it wants do allocate blind. The only case of fixed addresses (Addr!=nullptr) code like this should ever be using is allocating pages within a larger region that itself was randomly placed (which is what allocareVmar creates).

93

I think this should be a DCHECK, since if it's violated the next line is just going to crash anyway so no need to bloat optimized-with-fewer-assertions builds with a nicer check. It wouldn't hurt to make it stronger: !Addr == !!Data or the like. This code could also use more comments to make it clear what's going on. There are three distinct modes of calling this function (it really should be three functions, but was shoe-horned into a single call that is close POSIX mmap semantics so the linux.cpp version is a thin wrapper on mmap):

1. MAP_NOACCESS to reserve a region of address space, and update *Data to describe that region (this is where we call allocateVmar at the top).
2. Addr!=nullptr with a page-aligned address range inside the region returned by a previous MAP_NOACCESS call, where *Data must be the same info from that allocation.
3. Addr==nullptr, Data==nullptr for an immediate allocation (both address space and writable pages) that won't later be subdivided, just unmapped whole so there's no need for tracking anything.

This would be a lot cleaner as three separate functions in the platform layer here. The POSIX (linux.cpp) implementation would have three functions that all just call mmap pretty similarly. But in the current Fuchsia implementation, these are three thoroughly different things that would more naturally share very little in any common subroutine. (The shared bit is just mapping from a VMO, where in #2 that's choosing a fixed portion of the previously-created VMO to correspond to a portion of the previously-created VMAR, while in #3 it's just creating a new VMO and using the root VMAR with map-anywhere flags. So there's barely any commonality in the zx_vmar_map call being made.)

[abrachet]

compiler-rt/lib/scudo/standalone/fuchsia.cpp

60

Thank you for the explanation

93

I've just gone with a DCHECK for now, and then in a separate patch I intend to split this function out between 2 and 3 to document better what they do from the comment above here, but just have this function call them, not change the map api.

compiler-rt/lib/scudo/standalone/fuchsia.cpp

void *map(void *Addr, uptr Size, const char *Name, uptr Flags,
           MapPlatformData *Data) {
   DCHECK_EQ(Size % getPageSizeCached(), 0);
   const bool AllowNoMem = !!(Flags & MAP_ALLOWNOMEM);
 
   // For MAP_NOACCESS, just allocate a Vmar and return.
   if (Flags & MAP_NOACCESS)
     return allocateVmar(Size, Data, AllowNoMem);
 
   const zx_handle_t Vmar = Data ? Data->Vmar : _zx_vmar_root_self();
   CHECK_NE(Vmar, ZX_HANDLE_INVALID);
 
   zx_status_t Status;
   zx_handle_t Vmo;
   uint64_t VmoSize = 0;
   if (Data && Data->Vmo != ZX_HANDLE_INVALID) {
     // If a Vmo was specified, it's a resize operation.
     CHECK(Addr);
if (UNLIKELY(Status != ZX_OK)) {
       return nullptr;
     }
     _zx_object_set_property(Vmo, ZX_PROP_NAME, Name, strlen(Name));
   }
 
   uintptr_t P;
   zx_vm_option_t MapFlags =
       ZX_VM_PERM_READ | ZX_VM_PERM_WRITE | ZX_VM_ALLOW_FAULTS;
+  if (Addr)
+    DCHECK(Data);
   const uint64_t Offset =
       Addr ? reinterpret_cast<uintptr_t>(Addr) - Data->VmarBase : 0;
   if (Offset)
     MapFlags |= ZX_VM_SPECIFIC;
   Status = _zx_vmar_map(Vmar, MapFlags, Offset, Vmo, VmoSize, Size, &P);
   // No need to track the Vmo if we don't intend on resizing it. Close it.
   if (Flags & MAP_RESIZABLE) {
     DCHECK(Data);