[compiler-rt] [TySan] Fix false positives with derived classes (PR #126260)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 22 09:02:13 PDT 2025
https://github.com/gbMattN updated https://github.com/llvm/llvm-project/pull/126260
>From 0a94da3186ae40a9a8999d4b3f8e0f47643fce42 Mon Sep 17 00:00:00 2001
From: gbMattN <matthew.nagy at sony.com>
Date: Fri, 7 Feb 2025 16:33:01 +0000
Subject: [PATCH 1/4] [TySan] Fix false positives with derrived classes
---
compiler-rt/lib/tysan/tysan.cpp | 60 +++++++++++++------
.../tysan/derrived_default_constructor.cpp | 27 +++++++++
compiler-rt/test/tysan/inherited_member.cpp | 23 +++++++
3 files changed, 92 insertions(+), 18 deletions(-)
create mode 100644 compiler-rt/test/tysan/derrived_default_constructor.cpp
create mode 100644 compiler-rt/test/tysan/inherited_member.cpp
diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index f0230df9260e3..cf2b7d6bad564 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 0000000000000..b232d7949397c
--- /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 0000000000000..9a96f6f400213
--- /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;
+}
>From ba242d87b3d182d51696c550161291c6265f0856 Mon Sep 17 00:00:00 2001
From: gbMattN <matthew.nagy at sony.com>
Date: Tue, 4 Mar 2025 17:22:27 +0000
Subject: [PATCH 2/4] formating
---
compiler-rt/lib/tysan/tysan.cpp | 18 +++++++--------
.../tysan/derrived_default_constructor.cpp | 22 +++++++++----------
compiler-rt/test/tysan/inherited_member.cpp | 20 ++++++++---------
3 files changed, 28 insertions(+), 32 deletions(-)
diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index cf2b7d6bad564..1c1580e193d3b 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -102,10 +102,8 @@ static tysan_type_descriptor *getRootTD(tysan_type_descriptor *TD) {
return RootTD;
}
-bool walkAliasTree(
- tysan_type_descriptor* TDA, tysan_type_descriptor* TDB,
- uptr OffsetA, uptr OffsetB
-){
+bool walkAliasTree(tysan_type_descriptor *TDA, tysan_type_descriptor *TDB,
+ uptr OffsetA, uptr OffsetB) {
do {
if (TDA == TDB)
return OffsetA == OffsetB;
@@ -160,17 +158,19 @@ static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
return walkAliasTree(TDA, TDB, OffsetA, OffsetB);
}
-static bool isAliasingLegalWithOffset(tysan_type_descriptor *AccessTD, tysan_type_descriptor *ShadowTD, int OffsetInShadow){
+static bool isAliasingLegalWithOffset(tysan_type_descriptor *AccessTD,
+ tysan_type_descriptor *ShadowTD,
+ int OffsetInShadow) {
// This is handled in the other cases
- if(OffsetInShadow == 0)
+ if (OffsetInShadow == 0)
return false;
-
+
// You can't have an offset into a member
- if(ShadowTD->Tag == TYSAN_MEMBER_TD)
+ if (ShadowTD->Tag == TYSAN_MEMBER_TD)
return false;
int OffsetInAccess = 0;
- if(AccessTD->Tag == TYSAN_MEMBER_TD){
+ if (AccessTD->Tag == TYSAN_MEMBER_TD) {
OffsetInAccess = AccessTD->Member.Offset;
AccessTD = AccessTD->Member.Base;
}
diff --git a/compiler-rt/test/tysan/derrived_default_constructor.cpp b/compiler-rt/test/tysan/derrived_default_constructor.cpp
index b232d7949397c..39705bfbb3eae 100644
--- a/compiler-rt/test/tysan/derrived_default_constructor.cpp
+++ b/compiler-rt/test/tysan/derrived_default_constructor.cpp
@@ -2,26 +2,24 @@
#include <stdio.h>
-class Inner{
+class Inner {
public:
- void* ptr = nullptr;
+ void *ptr = nullptr;
};
-class Base{
+class Base {
public:
- void* buffer1;
- Inner inside;
- void* buffer2;
+ void *buffer1;
+ Inner inside;
+ void *buffer2;
};
-class Derrived : public Base{
-
-};
+class Derrived : public Base {};
Derrived derr;
-int main(){
- printf("%p", derr.inside.ptr);
+int main() {
+ printf("%p", derr.inside.ptr);
- return 0;
+ return 0;
}
diff --git a/compiler-rt/test/tysan/inherited_member.cpp b/compiler-rt/test/tysan/inherited_member.cpp
index 9a96f6f400213..f23d69e6c8065 100644
--- a/compiler-rt/test/tysan/inherited_member.cpp
+++ b/compiler-rt/test/tysan/inherited_member.cpp
@@ -2,22 +2,20 @@
#include <stdio.h>
-class Base{
+class Base {
public:
- void* first;
- void* second;
- void* third;
+ void *first;
+ void *second;
+ void *third;
};
-class Derrived : public Base{
-
-};
+class Derrived : public Base {};
Derrived derr;
-int main(){
- derr.second = nullptr;
- printf("%p", derr.second);
+int main() {
+ derr.second = nullptr;
+ printf("%p", derr.second);
- return 0;
+ return 0;
}
>From 0f309d68a46917567ae0e0f800e12dfea6a98188 Mon Sep 17 00:00:00 2001
From: gbMattN <matthew.nagy at sony.com>
Date: Tue, 22 Apr 2025 16:38:51 +0100
Subject: [PATCH 3/4] Moved call location
---
compiler-rt/lib/tysan/tysan.cpp | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index 1c1580e193d3b..8346b8f70ffaa 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -179,7 +179,8 @@ static bool isAliasingLegalWithOffset(tysan_type_descriptor *AccessTD,
}
static bool isAliasingLegal(tysan_type_descriptor *TDA,
- tysan_type_descriptor *TDB) {
+ tysan_type_descriptor *TDB,
+ int OffsetB = 0) {
if (TDA == TDB || !TDB || !TDA)
return true;
@@ -190,7 +191,8 @@ 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) || isAliasingLegalUp(TDB, TDA);
+ return isAliasingLegalUp(TDA, TDB) || isAliasingLegalUp(TDB, TDA) ||
+ isAliasingLegalWithOffset(TDA, TDB, OffsetB);
}
namespace __tysan {
@@ -267,7 +269,7 @@ __tysan_check(void *addr, int size, tysan_type_descriptor *td, int flags) {
OldTDPtr -= i;
OldTD = *OldTDPtr;
- if (!isAliasingLegal(td, OldTD) && !isAliasingLegalWithOffset(td, OldTD, i))
+ if (!isAliasingLegal(td, OldTD, i))
reportError(addr, size, td, OldTD, AccessStr,
"accesses part of an existing object", -i, pc, bp, sp);
>From 11e21b7e25306c0bbe6bf44fa2a077a1a79a0f7e Mon Sep 17 00:00:00 2001
From: gbMattN <matthew.nagy at sony.com>
Date: Tue, 22 Apr 2025 17:02:02 +0100
Subject: [PATCH 4/4] Added comments to explain why we treat offsets in shadow
memory opposite te to other accesses
---
compiler-rt/lib/tysan/tysan.cpp | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index 8346b8f70ffaa..e572d5324bc49 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -102,6 +102,7 @@ static tysan_type_descriptor *getRootTD(tysan_type_descriptor *TD) {
return RootTD;
}
+// Walk up TDA to see if it reaches TDB
bool walkAliasTree(tysan_type_descriptor *TDA, tysan_type_descriptor *TDB,
uptr OffsetA, uptr OffsetB) {
do {
@@ -158,24 +159,29 @@ static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
return walkAliasTree(TDA, TDB, OffsetA, OffsetB);
}
-static bool isAliasingLegalWithOffset(tysan_type_descriptor *AccessTD,
- tysan_type_descriptor *ShadowTD,
- int OffsetInShadow) {
+static bool isAliasingLegalWithOffset(tysan_type_descriptor *TDA,
+ tysan_type_descriptor *TDB,
+ int OffsetB) {
// This is handled in the other cases
- if (OffsetInShadow == 0)
+ if (OffsetB == 0)
return false;
// You can't have an offset into a member
- if (ShadowTD->Tag == TYSAN_MEMBER_TD)
+ if (TDB->Tag == TYSAN_MEMBER_TD)
return false;
- int OffsetInAccess = 0;
- if (AccessTD->Tag == TYSAN_MEMBER_TD) {
- OffsetInAccess = AccessTD->Member.Offset;
- AccessTD = AccessTD->Member.Base;
+ int OffsetA = 0;
+ if (TDA->Tag == TYSAN_MEMBER_TD) {
+ OffsetA = TDA->Member.Offset;
+ TDA = TDA->Member.Base;
}
- return walkAliasTree(ShadowTD, AccessTD, OffsetInShadow, OffsetInAccess);
+ // Since the access was partially inside TDB (the shadow), it can be assumed
+ // that we are accessing a member in an object. This means that rather than
+ // walk up the scalar access TDA to reach an object, we should walk up the
+ // object TBD to reach the scalar we are accessing it with. The offsets will
+ // still be checked at the end to make sure this alias is legal.
+ return walkAliasTree(TDB, TDA, OffsetB, OffsetA);
}
static bool isAliasingLegal(tysan_type_descriptor *TDA,
More information about the llvm-commits
mailing list