[PATCH] D20084: [sanitizer] Initial implementation of a Hardened Allocator

[Kostya Serebryany via llvm-commits]

kcc added inline comments.

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:115
@@ +114,3 @@
+  // CRC32 checksum of the Chunk pointer and its ChunkHeader.
+  // It currently uses the Intel Nehalem SSE4.2 crc32 64-bit instruction.
+  // TODO(kostyak): use a BSD checksum for the non-sse4.2 processors?
----------------
glider wrote:
> I would've started with a handwritten crc32 implementation and bother with hardware support only iff it's performance-critical (don't think it is)
I am pretty sure it *is* performance critical.
If this gets used on older or non-x86 systems we can add other implementation later. 

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:175
@@ +174,3 @@
+
+static void teardownThread(void *p) {
+  uptr v = reinterpret_cast<uptr>(p);
----------------
glider wrote:
> Please remind if Scudo is going to be used together with any of the sanitizers.
> If yes, the destructor magic won't probably work as intended, because other tools also play with it.
Afiact, scudo will not be combinable with any of the sanitizers other than with ubsan

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:262
@@ +261,3 @@
+struct Allocator {
+  static const uptr MaxAllowedMallocSize = 1ULL << 40;
+  static const uptr MaxThreadLocalQuarantine = 1U << 20;
----------------
filcab wrote:
> glider wrote:
> > Are we going to target 32-bit systems? This is gonna overflow uptr on x86.
> If this ends up being ported, it's a simple matter of using the `FIRST_32_SECOND_64` macro.
no 32-bit for now (or ever?)

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:1
@@ +1,2 @@
+//===-- scudo_allocator.h ---------------------------------------*- C++ -*-===//
+//
----------------
glider wrote:
> I don't insist much, but I think either the library name should be "scudo" instead of "hardened_allocator", or the names of the files under hardened_allocator/ should start with "hardened_allocator_"
I initially proposed to name the dir hardened_allocator to make it more self-descriptive.
But if others don't mind to have dir named "scuda" let the author decide. 


http://reviews.llvm.org/D20084