Skip to content

[TySan] Fix false positives with derived classes #126260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 49 additions & 17 deletions compiler-rt/lib/tysan/tysan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

// Walk up TDA to see if it reaches TDB.
static bool walkAliasTree(tysan_type_descriptor *TDA,
tysan_type_descriptor *TDB, uptr OffsetA,
uptr OffsetB) {
do {
if (TDA == TDB)
return OffsetA == OffsetB;
Expand Down Expand Up @@ -153,8 +143,50 @@ static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
return false;
}

// Walk up the tree starting with TDA to see if we reach TDB.
static bool isAliasingLegalUp(tysan_type_descriptor *TDA,
tysan_type_descriptor *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 *TDA,
tysan_type_descriptor *TDB,
uptr OffsetB) {
// This is handled by calls to isAliasingLegalUp.
if (OffsetB == 0)
return false;

// You can't have an offset into a member.
if (TDB->Tag == TYSAN_MEMBER_TD)
return false;

uptr OffsetA = 0;
if (TDA->Tag == TYSAN_MEMBER_TD) {
OffsetA = TDA->Member.Offset;
TDA = TDA->Member.Base;
}

// 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,
tysan_type_descriptor *TDB, int TDAOffset = 0) {
tysan_type_descriptor *TDB, uptr OffsetB = 0) {
if (TDA == TDB || !TDB || !TDA)
return true;

Expand All @@ -165,8 +197,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, 0) || isAliasingLegalUp(TDB, TDA, 0) ||
isAliasingLegalUp(TDA, TDB, TDAOffset);
return isAliasingLegalUp(TDA, TDB) || isAliasingLegalUp(TDB, TDA) ||
isAliasingLegalWithOffset(TDA, TDB, OffsetB);
}

namespace __tysan {
Expand Down
25 changes: 25 additions & 0 deletions compiler-rt/test/tysan/derrived_default_constructor.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// 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;
}
21 changes: 21 additions & 0 deletions compiler-rt/test/tysan/inherited_member.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// 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;
Comment on lines +6 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

class Derrived : public Base {};

Derrived derr;

int main() {
derr.second = nullptr;
printf("%p", derr.second);

return 0;
}
Loading