[compiler-rt] r340633 - [scudo] Replace eraseHeader with compareExchangeHeader for Quarantined chunks

[Kostya Kortchinsky via llvm-commits]

Author: cryptoad
Date: Fri Aug 24 11:21:32 2018
New Revision: 340633

URL: http://llvm.org/viewvc/llvm-project?rev=340633&view=rev
Log:
[scudo] Replace eraseHeader with compareExchangeHeader for Quarantined chunks

Summary:
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 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.

Reviewers: alekseyshl, flowerhack, eugenis

Reviewed By: eugenis

Subscribers: delcypher, jfb, #sanitizers, llvm-commits

Differential Revision: https://reviews.llvm.org/D51224

Modified:
    compiler-rt/trunk/lib/scudo/scudo_allocator.cpp

Modified: compiler-rt/trunk/lib/scudo/scudo_allocator.cpp
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/scudo/scudo_allocator.cpp?rev=340633&r1=340632&r2=340633&view=diff
==============================================================================
--- compiler-rt/trunk/lib/scudo/scudo_allocator.cpp (original)
+++ compiler-rt/trunk/lib/scudo/scudo_allocator.cpp Fri Aug 24 11:21:32 2018
@@ -129,16 +129,9 @@ namespace Chunk {
             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 @@ struct QuarantineCallback {
     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);