[PATCH] D53789: [hwasan] optionally right-align heap allocations

[Kostya Serebryany via Phabricator via llvm-commits]

kcc added inline comments.


================
Comment at: lib/hwasan/hwasan_allocator.cc:136
 
+  if (orig_size % kShadowAlignment) {
+    if (int malloc_align_right = flags()->malloc_align_right) {
----------------
dvyukov wrote:
> eugenis wrote:
> > dvyukov wrote:
> > > Shouldn't we also look at user-requested alignment here? It looks like we only satisfy natural alignment.
> > Yeah, at least for calls like posix_memalign.
> > 
> > This mode will break some things anyway, ex.:
> > struct S {
> >   int count;
> >   char name[0];
> > };
> > 
> > When allocated with malloc(sizeof(S) + count) the size will not be a multiple of alignment, so "count" will be misaligned. That's why this mode can not be on by default. Let's hope it is not a common case.
> > 
> Why will not respecting alignment argument help? That's the alignment that we ensure in normal mode.
I've changed the code so that it respects alignment > 16. 
Respecting smaller alignment is harder because the implecit alignment requirement for malloc is 16
(it's a bit more complicated than that, actually, I don't think the standard actually tells what the alignment is, 
it just tells that it should be sufficient for any builtin type)

This mode is unlikely to be enabled by default in its current form. 
For we can only tell what modifications will be needed once we try. 


================
Comment at: lib/hwasan/hwasan_allocator.cc:139
+      uptr as_uptr = reinterpret_cast<uptr>(user_ptr);
+      if (malloc_align_right == 2    // always right-align
+          || as_uptr & (1 << 20)) {  // use an ASLR bit as a random choice.
----------------
eugenis wrote:
> Use named constants, like kHandleSignalYes.
done


================
Comment at: lib/hwasan/hwasan_allocator.cc:140
+      if (malloc_align_right == 2    // always right-align
+          || as_uptr & (1 << 20)) {  // use an ASLR bit as a random choice.
+        user_ptr = reinterpret_cast<void *>(AlignRight(as_uptr, orig_size));
----------------
eugenis wrote:
> This is not very random - entire region will have the same bit, so long running programs are likely to be stuck in either direction.
> 
> Use HwasanThread::random_buffer_ instead?
> 
Changed to use a bit of the tag, that is random (unless, of course, we disable random tags via a flag)


================
Comment at: test/hwasan/TestCases/heap-buffer-overflow.c:28
   x[offset] = 42;
-// CHECK40: allocated heap chunk; size: 32 offset: 8
+// CHECK40: allocated heap chunk; size: 32 offset: {{8|10}}
 // CHECK40: is located 10 bytes to the right of 30-byte region
----------------
dvyukov wrote:
> This won't catch regressions as it will be happy with 8 in all cases.
done


================
Comment at: test/hwasan/TestCases/heap-buffer-overflow.c:31
 //
-// CHECK80: allocated heap chunk; size: 32 offset: 16
+// CHECK80: allocated heap chunk; size: 32 offset: {{16|18}}
 // CHECK80: is located 50 bytes to the right of 30-byte region
----------------
dvyukov wrote:
> Ditto.
done


================
Comment at: test/hwasan/TestCases/heap-buffer-overflow.c:34
 //
-// CHECKm30: allocated heap chunk; size: 32 offset: 2
+// CHECKm30: allocated heap chunk; size: 32 offset: {{2|4}}
 // CHECKm30: is located 30 bytes to the left of 30-byte region
----------------
dvyukov wrote:
> Ditto.
done


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D53789