[PATCH] D40754: [sanitizer] 64-bit allocator's PopulateFreeArray partial refactor

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl accepted this revision.
alekseyshl added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary64.h:723
+      region->mapped_meta += meta_map_size;
     }
 
----------------
cryptoad wrote:
> alekseyshl wrote:
> > cryptoad wrote:
> > > alekseyshl wrote:
> > > > I'd suggest to wrap the entire metadata allocating block (from // Calculate the required space for metadata) with:
> > > > 
> > > >   if (kMetadataSize) {
> > > >     ...
> > > >   }
> > > So I thought about it, but I was wondering about something: currently the check for exhaustion will be done whether or not we use metadata.
> > > I wanted to leave it like that at first, because it sort of made sense.
> > > But then I figure that if we are not using metadata, and are exhausted, then the initial mmap would fail anyway since we'd hit the free array.
> > > It seems to be safe to place all of this within `if (kMetadataSize)`, but I wanted to raise this to your attention and get your feedback.
> > Yep, now we will return false due to MapWithCallback failure and the OOM error handling will kick in. I think, it's just what we want.
> Tried out wrapping everything within a `if (kMetadataSize)`.
> The `SizeClassAllocator64PopulateFreeListOOM` test fails.
> If we don't keep the exhaustion check out of the if, the map is successful, and we "overflow" into the free array space (it's empty and mappable).
> So I think the current way is the way to go.
That means, when kMetadataSize == 0 and exhaustion check is true, we already stepped into the free array zone, right? We have not used any of it yet, but still, sloppy. Ok, let's take care of it in a separate patch.


https://reviews.llvm.org/D40754