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

[Mike Aizatsky via llvm-commits]

aizatsky added inline comments.

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:123
@@ +122,3 @@
+    crc = _mm_crc32_u64(crc, header_holder[1]);
+    return static_cast<u16>(crc ^ cookie);
+  }
----------------
This uses this.cookie. Is this intended?

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:127
@@ +126,3 @@
+  // Loads and unpacks the header, verifying the checksum in the process.
+  void LoadHeader(UnpackedHeader *unpacked_header) {
+    PackedHeader packed_header;
----------------
const function?

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:129
@@ +128,3 @@
+    PackedHeader packed_header;
+    __atomic_load(reinterpret_cast<u128 *>(this), &packed_header,
+                  __ATOMIC_ACQUIRE);
----------------
I suggest to use PackedHeader instead of u128 throught the source code. Will make maintaining/porting much easier.

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:141
@@ +140,3 @@
+  // A different one would mean that another thread would have raced us.
+  void StoreHeader(UnpackedHeader *new_unpacked_header,
+                   UnpackedHeader *old_unpacked_header) {
----------------
Instead of having optional parameter, maybe its better to split this function into two? One simple copying, another - verify and copy.

================
Comment at: lib/hardened_allocator/scudo_allocator.h:62
@@ +61,3 @@
+  void SetFrom(const Flags *f, const CommonFlags *cf);
+  void CopyTo(Flags *f, CommonFlags *cf);
+};
----------------
const function?

================
Comment at: lib/hardened_allocator/scudo_allocator.h:68
@@ +67,3 @@
+
+const uptr kAllocatorSpace = ~0ULL;
+const uptr kAllocatorSize  =  0x10000000000ULL;
----------------
static?


http://reviews.llvm.org/D20084