[PATCH] D51224: [scudo] Replace eraseHeader with compareExchangeHeader for Quarantined chunks

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad created this revision.
cryptoad added reviewers: alekseyshl, flowerhack, eugenis.
Herald added subscribers: Sanitizers, jfb, delcypher.

The reason for the existence of `eraseHeader` was that it was deemed faster
to null-out a chunk header, effectively making it invalid, rather than marking
it as available, which incurred a checksum computation and a cmpxchg.

A previous use of `eraseHeader` was removed with https://reviews.llvm.org/D50655 due to a race.

Now we remove the second use of it in the Quarantine deallocation path and
replace is with a `compareExchangeHeader`.

The reason for this is that greatly helps debugging some heap bugs as the chunk
header is now valid and the chunk marked available, as opposed to the header
being invalid. Eg: we get an invalid state error, instead of an invalid header
error, which reduces the possibilities. The computational penalty is negligible.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D51224

Files:
  lib/scudo/scudo_allocator.cpp


Index: lib/scudo/scudo_allocator.cpp
===================================================================
--- lib/scudo/scudo_allocator.cpp
+++ lib/scudo/scudo_allocator.cpp
@@ -129,16 +129,9 @@
             computeChecksum(Ptr, &NewUnpackedHeader));
   }
 
-  // Nulls out a chunk header. When returning the chunk to the backend, there
-  // is no need to store a valid ChunkAvailable header, as this would be
-  // computationally expensive. Zeroing out serves the same purpose by making
-  // the header invalid. In the extremely rare event where 0 would be a valid
-  // checksum for the chunk, the state of the chunk is ChunkAvailable anyway.
+  // Ensure that ChunkAvailable is 0, so that if a 0 checksum is ever valid
+  // for a fully nulled out header, its state will be available anyway.
   COMPILER_CHECK(ChunkAvailable == 0);
-  static INLINE void eraseHeader(void *Ptr) {
-    const PackedHeader NullPackedHeader = 0;
-    atomic_store_relaxed(getAtomicHeader(Ptr), NullPackedHeader);
-  }
 
   // Loads and unpacks the header, verifying the checksum in the process.
   static INLINE
@@ -185,7 +178,9 @@
     Chunk::loadHeader(Ptr, &Header);
     if (UNLIKELY(Header.State != ChunkQuarantine))
       dieWithMessage("invalid chunk state when recycling address %p\n", Ptr);
-    Chunk::eraseHeader(Ptr);
+    UnpackedHeader NewHeader = Header;
+    NewHeader.State = ChunkAvailable;
+    Chunk::compareExchangeHeader(Ptr, &NewHeader, &Header);
     void *BackendPtr = Chunk::getBackendPtr(Ptr, &Header);
     if (Header.ClassId)
       getBackend().deallocatePrimary(Cache_, BackendPtr, Header.ClassId);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51224.162411.patch
Type: text/x-patch
Size: 1615 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180824/197e83e8/attachment.bin>