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

[Kostya Serebryany via llvm-commits]

kcc added a comment.

Mostly LG. 
Please address a few remaining nits and wait until tomorrow for more comments. 
If no significant comments, rename the dir to scudo and I will land it.

Note: I did not review this code from security perspective in details because

- not an expert
- there are known security weaknesses in the backend allocator (we will need to handle them separately)
- it'll be easier to do further security assessment once the code is committed.


================
Comment at: docs/HardenedAllocator.rst:89
@@ +88,3 @@
+Your linked binary should now make use of the Scudo allocation and deallocation
+functions.
+
----------------
Did you? 

================
Comment at: docs/HardenedAllocator.rst:94
@@ +93,3 @@
+Several aspects of the allocator can be configured through environment options,
+following the usual ASan options syntax, through the variable SCUDO_OPTIONS.
+
----------------
Give an example instead of referring to "usual ASan syntax".
Scudo users don't have to be asan experts. 

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:63
@@ +62,3 @@
+  // 2nd 8 bytes
+  u64 Offset        : 20; // Offset from the beginning of the backend
+                           // allocation to the beginning chunk itself, in
----------------
align the comment block

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:109
@@ +108,3 @@
+        (NewUnpackedHeader->Checksum != Checksum(NewUnpackedHeader))) {
+      Printf("ERROR: corrupted chunk header at address %p\n", this);
+      Die();
----------------
I suggest to replace all cases of 
   if (!cond) {
     Printf()
     Die()
   }
With 
    if (!cond)
      DieWithMessage();


This is using the Printf from sanitizer_common, right? 
It might be worth replacing it with your own, simpler one. 
If you agree, just leave a TODO near DieWithMessage and address it later. 

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:278
@@ +277,3 @@
+  void init(const AllocatorOptions &Options) {
+    // Currently SSE 4.2 support is required. This might change late.
+    CHECK(testCPUFeature(SSE4_2)); // for crc32
----------------
s/late/later

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:18
@@ +17,3 @@
+#ifndef __x86_64__
+# error "The Scudo hardened allocator currently only supports on x86_64."
+#endif
----------------
is currently only supported? 
os "supports x86_64"? 

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:49
@@ +48,3 @@
+
+const uptr AllocatorSpace = ~0ULL;
+const uptr AllocatorSize  =  0x10000000000ULL;
----------------
Does the following block of code have to be in this header? 
Why not in .cc? 


http://reviews.llvm.org/D20084