[TySan] Fix false positives with derived classes

[gbMattN]

Fixes issue #125079 as well as another case I discovered while trying to build LLVM with TySan.
The case when the access has an offset needs some slightly different legwork to the average check.

[llvmbot]

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

Author: None (gbMattN)

Changes

Fixes issue #125079 as well as another case I discovered while trying to build LLVM with TySan.
The case when your access has an offset needs some slightly different legwork to the average check.

---

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

3 Files Affected:

• (modified) compiler-rt/lib/tysan/tysan.cpp (+42-18)
• (added) compiler-rt/test/tysan/derrived_default_constructor.cpp (+27)
• (added) compiler-rt/test/tysan/inherited_member.cpp (+23)

diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index f0230df9260e3a1..cf2b7d6bad564ec 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -102,20 +102,10 @@ static tysan_type_descriptor *getRootTD(tysan_type_descriptor *TD) {
   return RootTD;
 }
 
-static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
-                              tysan_type_descriptor *TDB, int TDAOffset) {
-  // Walk up the tree starting with TDA to see if we reach TDB.
-  uptr OffsetA = 0, OffsetB = 0;
-  if (TDB->Tag == TYSAN_MEMBER_TD) {
-    OffsetB = TDB->Member.Offset;
-    TDB = TDB->Member.Base;
-  }
-
-  if (TDA->Tag == TYSAN_MEMBER_TD) {
-    OffsetA = TDA->Member.Offset - TDAOffset;
-    TDA = TDA->Member.Base;
-  }
-
+bool walkAliasTree(
+  tysan_type_descriptor* TDA, tysan_type_descriptor* TDB,
+  uptr OffsetA, uptr OffsetB
+){
   do {
     if (TDA == TDB)
       return OffsetA == OffsetB;
@@ -153,8 +143,43 @@ static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
   return false;
 }
 
+static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
+                              tysan_type_descriptor *TDB) {
+  // Walk up the tree starting with TDA to see if we reach TDB.
+  uptr OffsetA = 0, OffsetB = 0;
+  if (TDB->Tag == TYSAN_MEMBER_TD) {
+    OffsetB = TDB->Member.Offset;
+    TDB = TDB->Member.Base;
+  }
+
+  if (TDA->Tag == TYSAN_MEMBER_TD) {
+    OffsetA = TDA->Member.Offset;
+    TDA = TDA->Member.Base;
+  }
+
+  return walkAliasTree(TDA, TDB, OffsetA, OffsetB);
+}
+
+static bool isAliasingLegalWithOffset(tysan_type_descriptor *AccessTD, tysan_type_descriptor *ShadowTD, int OffsetInShadow){
+  // This is handled in the other cases
+  if(OffsetInShadow == 0)
+    return false;
+  
+  // You can't have an offset into a member
+  if(ShadowTD->Tag == TYSAN_MEMBER_TD)
+    return false;
+
+  int OffsetInAccess = 0;
+  if(AccessTD->Tag == TYSAN_MEMBER_TD){
+    OffsetInAccess = AccessTD->Member.Offset;
+    AccessTD = AccessTD->Member.Base;
+  }
+
+  return walkAliasTree(ShadowTD, AccessTD, OffsetInShadow, OffsetInAccess);
+}
+
 static bool isAliasingLegal(tysan_type_descriptor *TDA,
-                            tysan_type_descriptor *TDB, int TDAOffset = 0) {
+                            tysan_type_descriptor *TDB) {
   if (TDA == TDB || !TDB || !TDA)
     return true;
 
@@ -165,8 +190,7 @@ static bool isAliasingLegal(tysan_type_descriptor *TDA,
   // TDB may have been adjusted by offset TDAOffset in the caller to point to
   // the outer type. Check for aliasing with and without adjusting for this
   // offset.
-  return isAliasingLegalUp(TDA, TDB, 0) || isAliasingLegalUp(TDB, TDA, 0) ||
-         isAliasingLegalUp(TDA, TDB, TDAOffset);
+  return isAliasingLegalUp(TDA, TDB) || isAliasingLegalUp(TDB, TDA);
 }
 
 namespace __tysan {
@@ -243,7 +267,7 @@ __tysan_check(void *addr, int size, tysan_type_descriptor *td, int flags) {
     OldTDPtr -= i;
     OldTD = *OldTDPtr;
 
-    if (!isAliasingLegal(td, OldTD, i))
+    if (!isAliasingLegal(td, OldTD) && !isAliasingLegalWithOffset(td, OldTD, i))
       reportError(addr, size, td, OldTD, AccessStr,
                   "accesses part of an existing object", -i, pc, bp, sp);
 
diff --git a/compiler-rt/test/tysan/derrived_default_constructor.cpp b/compiler-rt/test/tysan/derrived_default_constructor.cpp
new file mode 100644
index 000000000000000..b232d7949397c35
--- /dev/null
+++ b/compiler-rt/test/tysan/derrived_default_constructor.cpp
@@ -0,0 +1,27 @@
+// RUN: %clangxx_tysan %s -o %t && %run %t 2>&1 | FileCheck --implicit-check-not ERROR %s
+
+#include <stdio.h>
+
+class Inner{
+public:
+    void* ptr = nullptr;
+};
+
+class Base{
+public:
+    void* buffer1;
+    Inner inside;
+    void* buffer2;
+};
+
+class Derrived : public Base{
+
+};
+
+Derrived derr;
+
+int main(){
+    printf("%p", derr.inside.ptr);
+
+    return 0;
+}
diff --git a/compiler-rt/test/tysan/inherited_member.cpp b/compiler-rt/test/tysan/inherited_member.cpp
new file mode 100644
index 000000000000000..9a96f6f40021391
--- /dev/null
+++ b/compiler-rt/test/tysan/inherited_member.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx_tysan %s -o %t && %run %t 2>&1 | FileCheck --implicit-check-not ERROR %s
+
+#include <stdio.h>
+
+class Base{
+public:
+    void* first;
+    void* second;
+    void* third;
+};
+
+class Derrived : public Base{
+
+};
+
+Derrived derr;
+
+int main(){
+    derr.second = nullptr;
+    printf("%p", derr.second);
+
+    return 0;
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[gbMattN]

@fhahn nudge for review

[gbMattN]

@fhahn Do you think it would be possible to get this into a release candidate before llvm20 releases?

[gbMattN]

@fhahn @vitalybuka @MaskRay

[gbMattN]

@fhahn

[fhahn]

Fixes issue #125079 as well as another case I discovered while trying to build LLVM with TySan.
The case when the access has an offset needs some slightly different legwork to the average check.

It would be great if you could spell out the differences, and ideally also make this clear in the code comments.

[fhahn]

Should still be static?

Suggested change

	bool walkAliasTree(tysan_type_descriptor *TDA, tysan_type_descriptor *TDB,
	static bool walkAliasTree(tysan_type_descriptor *TDA, tysan_type_descriptor *TDB,

[fhahn]

still pending?

[gbMattN]

Ah, was too liberal in my git reset --hard's I think 😅

[gbMattN]

It would be great if you could spell out the differences, and ideally also make this clear in the code comments.

I've added a comment into the code to explain the rational. I will also explain it here.
When you walk up the alias tree, the TDs go from structs to members. If you are accessing a TD in shadow with an offset, you have either an access violation, or are accessing a large struct TD. If you walk up the alias tree of the type you are accessing with, you won't find that struct, since you are accessing it with a TD for a member variable inside said struct.

Therefore we should instead walk up the alias tree of this large struct to find the type we are accessing, and then check to see if the offset of that type in the struct is equal to the offset of our read in shadow memory. Since this method of checking is the opposite to the default way of checking, I've split it out into a separate function.

[fhahn]

still pending?

[fhahn]

LGTM, thanks

[fhahn]

Suggested change

	// Walk up TDA to see if it reaches TDB
	// Walk up TDA to see if it reaches TDB.

[fhahn]

Should the comment be moved outside function?

Suggested change

	static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
	tysan_type_descriptor *TDB) {
	// Walk up the tree starting with TDA to see if we reach TDB.
	// Walk up the tree starting with TDA to see if we reach TDB.
	static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
	tysan_type_descriptor *TDB) {

[fhahn]

Oh one thing I forgot was if we can add a test with a type violation that triggers with the new path? Or is that already covered with existing tests?

[gbMattN]

Its covered in basic.c when we partially access a short