[hwasan] Fix DCHECK with COMPILER_RT_DEBUG=ON

[arichardson]

It appears this assertion should actually be checking that the pointer
is aligned to sz bytes rather than the shadow alignment, since
otherwise the (ptr & (kShadowAlignment - 1)) mask would always be 0.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Alexander Richardson (arichardson)

Changes

It appears this assertion should actually be checking that the pointer
is aligned to sz bytes rather than the shadow alignment, since
otherwise the (ptr & (kShadowAlignment - 1)) mask would always be 0.

---

Full diff: https://github.com/llvm/llvm-project/pull/84612.diff

1 Files Affected:

• (modified) compiler-rt/lib/hwasan/hwasan_checks.h (+1-1)

diff --git a/compiler-rt/lib/hwasan/hwasan_checks.h b/compiler-rt/lib/hwasan/hwasan_checks.h
index 0911af30dcb8fc..7067e7140e7bef 100644
--- a/compiler-rt/lib/hwasan/hwasan_checks.h
+++ b/compiler-rt/lib/hwasan/hwasan_checks.h
@@ -140,7 +140,7 @@ __attribute__((always_inline, nodebug)) static inline uptr ShortTagSize(
 
 __attribute__((always_inline, nodebug)) static inline bool
 PossiblyShortTagMatches(tag_t mem_tag, uptr ptr, uptr sz) {
-  DCHECK(IsAligned(ptr, kShadowAlignment));
+  DCHECK(IsAligned(ptr, sz));
   tag_t ptr_tag = GetTagFromPointer(ptr);
   if (ptr_tag == mem_tag)
     return true;

[vitalybuka]

This change conflicts with CheckAddressSized @ line 186, where tail_sz can be anything

However can I see how this DCHECK fails for CheckAddress.

PossiblyShortTagMatches @ line

return *(u8 *)(ptr | (kShadowAlignment - 1)) == ptr_tag;

expects that ptr points to the first byte of the granule.

[vitalybuka]

I suppose this one should guaranty that we can't get here with wrong alignment

IRBuilder<> IRB(O.getInsn());
  if (!O.TypeStoreSize.isScalable() && isPowerOf2_64(O.TypeStoreSize) &&
      (O.TypeStoreSize / 8 <= (1ULL << (kNumberOfAccessSizes - 1))) &&
      (!O.Alignment || *O.Alignment >= Mapping.getObjectAlignment() ||
       *O.Alignment >= O.TypeStoreSize / 8)) {

But I am not sure how :)

[vitalybuka]

I re-read your description, good point about | (kShadowAlignment - 1)

DCHECK(IsAligned(ptr, sz)); is still not correct for CheckAddressSized. I guess we need to remove it.

[arichardson]

Thanks for looking into this, I am very unfamiliar with the hwasan code and was just trying to get the tests to pass. The always zero condition made me think this fix is correct and it would make sense for all accesses to be naturally aligned but I don't yet understand the short tag logic.

[vitalybuka]

yes, this means we don't care about pointer alignment in this function and we can remove DCHECK?

[fmayer]

Sorry, I got confused. I see no reason that we need this DCHECK, and yes it's clearly not legal for hwasan_load1, let's drop it?

[fmayer]

i went ahead and submitted 7ef1a59

[arichardson]

Fixed in 7ef1a59