[PATCH] D20084: [sanitizer] Initial implementation of a Hardened Allocator
Kostya Serebryany via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 1 10:19:12 PDT 2016
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
More information about the llvm-commits
mailing list