[PATCH] D87739: scudo: Add an API for disabling memory initialization per-thread.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:417
+            // therefore it needs to be zeroed now.
+            memset(TaggedPtr, 0, Min(Size, PrevEnd - TaggedUserPtr));
+          } else if (Size) {
----------------
pcc wrote:
> eugenis wrote:
> > eugenis wrote:
> > > resizeTaggedChunk does not zero memory. I think this  memset needs to cover the entire allocation?
> > Ah no, I misread. Ignore this.
> I think the problem is related to this line actually. `resizeTaggedChunk` rounds up to 16 before it starts zeroing, so the memset needs to stop at the rounded-up original bound, otherwise it can leave the end of the original last granule uninitialized. I am testing this diff:
> ```
> @@ -414,12 +424,18 @@ public:
>              // UAF tag. But if tagging was disabled per-thread when the memory
>              // was freed, it would not have been retagged and thus zeroed, and
>              // therefore it needs to be zeroed now.
> -            memset(TaggedPtr, 0, Min(Size, PrevEnd - TaggedUserPtr));
> +            memset(TaggedPtr, 0,
> +                   Min(Size, roundUpTo(PrevEnd - TaggedUserPtr,
> +                                       archMemoryTagGranuleSize())));
>            } else if (Size) {
>              // Clear any stack metadata that may have previously been stored in
>              // the chunk data.
> ```
Okay, this looks like this fixed the problem (stress tested on Android/FVP with a more rigorous version of my stress testing patch that verifies that calloc allocations are zeroed at allocation time). Also added a test case that fails with the previous version of my patch.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:496
     if (Options.DeallocTypeMismatch) {
-      if (Header.Origin != Origin) {
+      if (Header.OriginOrWasZeroed != Origin) {
         // With the exception of memalign'd chunks, that can be still be free'd.
----------------
hctim wrote:
> pcc wrote:
> > hctim wrote:
> > > Isn't this now broken under `dealloc_type_mismatch` and MTE?
> > No, because the field is intended to only have the "was zeroed" meaning while the chunk is not allocated. Note that the field is read before the call to `quarantineOrDeallocateChunk` later in this function causes it to take the "was zeroed" meaning.
> I see - thanks for the clarification.  Maybe `OriginIfAliveOrWasZeroedIfDead`? A bit wordy (can't think of anything more concise right now), but otherwise it's a little hard to infer this which meaning applies at which time (especially given that `SizeOrUnusedBytes` has different semantics). Either that or a comment in the declaration explaining the semantics would probably suffice.
Added a comment in the declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87739