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

[Vitaly Buka via llvm-commits]

vitalybuka added a comment.

There are some inconsistency with style http://llvm.org/docs/CodingStandards.html
also preferred file suffix is .cpp.
Could you please rename files only after the rest is accepted, just to simplify review?


================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:53
@@ +52,3 @@
+
+enum ChunkState : u8 {
+  CHUNK_AVAILABLE  = 0,
----------------
enum ChunkState : u8 {
  ChunkAvailible  = 0,
};

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:80
@@ +79,3 @@
+  // 1st 8 bytes
+  u128 checksum       : 16;
+  u128 requested_size : 40;
----------------
I see why you need 128bit for structure. 
Why do you need u128 type for 2 bit members?

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:309
@@ +308,3 @@
+    uptr needed_size = rounded_size + extra_bytes;
+    // CHECK_GE(needed_size, size); // Overflow cannot happen
+    if (needed_size >= kMaxAllowedMallocSize)
----------------
could be removed?

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:406
@@ +405,3 @@
+      Printf("ERROR: attempted to size a non-allocated chunk at address %p\n",
+             chunk);
+    }
----------------
no Die?

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:511
@@ +510,3 @@
+void *scudo_realloc(void *ptr, uptr size) {
+  if (ptr == nullptr)
+    return instance.Allocate(size, Allocator::kMinAlignment, FROM_MALLOC);
----------------
code sometimes uses if(p), sometimes if (p == nullptr)
I'd recommed if(p) and if(!p) everywhere for consistency

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:543
@@ +542,3 @@
+int scudo_posix_memalign(void **memptr, uptr alignment, uptr size) {
+  void *ptr = instance.Allocate(size, alignment, FROM_MEMALIGN);
+  *memptr = ptr;
----------------
maybe remove temp variable?

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:57
@@ +56,3 @@
+struct AllocatorOptions {
+  u32 quarantine_size_mb;
+  bool may_return_null;
----------------
QuarantineSizeMb;

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:70
@@ +69,3 @@
+
+const uptr kAllocatorSpace = ~0ULL;
+const uptr kAllocatorSize  =  0x10000000000ULL;
----------------
const uptr AllocatorSpace =

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:72
@@ +71,3 @@
+const uptr kAllocatorSize  =  0x10000000000ULL;
+static const uptr kMinAlignmentLog = 4; // 16 bytes for x64
+static const uptr kMaxAlignmentLog = 24;
----------------
static is not needed

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:83
@@ +82,3 @@
+
+void *scudo_malloc(uptr size, AllocType alloc_type);
+void scudo_free(void *ptr, AllocType alloc_type);
----------------
scudoMalloc(uptr Size, AllocType AllocType)

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_flags.h:27
@@ +26,3 @@
+
+extern Flags scudo_flags_dont_use_directly;
+inline Flags *flags() {
----------------
I see call only from init, so maybe this does not need to be inline
and extern Flags scudo_flags_dont_use_directly; can be moved into cc


http://reviews.llvm.org/D20084