[PATCH] D89993: [GWP-ASan] Refactor memory mapping functions

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.

LGTM.  Different names and more comments would be preferable.



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:128
+  // The pool memory is initially reserved and inaccessible, and RW mappings are
+  // subsequently created and destroyed via commitPool() and decommitPool().
+  // Each mapping is named on platforms that support it, primarily Android. This
----------------
hctim wrote:
> nits just to be really clear/descriptive with the function names.
> 
> `mapWithRW()`
> 
> `reserveGuardedPool()`
> `commitGuardedPoolRegionWithRW()`
> `decommitGuardedPoolRegion()`
> `unreserveGuardedPool()`
> 
> Also can you add a comment above why the pool is special? Something like:
> 
> ```
> The pool is managed separately, as some platforms (particularly Fuchsia) manage virtual memory regions as a chunk where individual pages can still have separate permissions. These platforms maintain metadata about the region in order to perform operations. The pool is unique as it's the only thing in GWP-ASan that treats pages in a single VM region on an individual basis for page protection.
> ``` 
I'm not in favor of the "decommit" name because the semantics on Fuchsia are to unmap the region so that later accesses will fault.  "decommit" usually means "restore to lazy zero-fill state", not "make potentially inaccessible".

I'm certainly in favor of any clarifying comments that anyone thinks help.




================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:135
+
+  void *reservePool(size_t Size);
+  void commitPool(void *Ptr, size_t Size) const;
----------------
I'd like to see some explicit comments here about the API contract: the returned ptr is the reserved address range of (at least) Size bytes; a Ptr/Size in commitPool must be a subrange of that range; a Ptr/Size in decommitPool must be an exact pair previously passed to commitPool.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:136
+  void *reservePool(size_t Size);
+  void commitPool(void *Ptr, size_t Size) const;
+  void decommitPool(void *Ptr, size_t Size) const;
----------------
How about allocateInPool and deallocateInPool?



================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp:28
 
-void MaybeSetMappingName(void *Mapping, size_t Size, const char *Name) {
+static void MaybeSetMappingName(void *Mapping, size_t Size, const char *Name) {
 #ifdef ANDROID
----------------
Google style is to prefer anonymous namespace, I don't know about LLVM style.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp:47
+                   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
   Check(Ptr != MAP_FAILED, "Failed to map guarded pool allocator memory");
   MaybeSetMappingName(Ptr, Size, Name);
----------------
could use the name in the error


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89993/new/

https://reviews.llvm.org/D89993