[TySan] Fixed false positive when accessing offset member variables

[gbMattN]

Original pull request here

This patch fixes a false positive bug in TySan. If you try access a member variable other than the first, TySan currently believes you are trying to access a type part-way through and reports an error.
In this patch we check to see if the type accessed has members, and if so searches through to find the internal type we are accessing.

[llvmbot]

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

Author: None (gbMattN)

Changes

Original pull request here

This patch fixes a false positive bug in TySan. If you try access a member variable other than the first, TySan currently believes you are trying to access a type part-way through and reports an error.
In this patch we check to see if the type accessed has members, and if so searches through to find the internal type we are accessing.

---

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

2 Files Affected:

• (modified) compiler-rt/lib/tysan/tysan.cpp (+24-1)
• (added) compiler-rt/test/tysan/global-struct-members.c (+30)

diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index 39d78e7c95e0cd..8595d9014d14e8 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -227,8 +227,31 @@ __tysan_check(void *addr, int size, tysan_type_descriptor *td, int flags) {
     int i = -((sptr)OldTD);
     OldTDPtr -= i;
     OldTD = *OldTDPtr;
+  
+    tysan_type_descriptor *AccessedType = OldTD;
+    
+    // Only check if we are accessing members if the type exists
+    if(OldTD != nullptr){
+      // When shadow memory is set for global objects, the entire object is tagged
+      // with the struct type This means that when you access a member variable,
+      // tysan reads that as you accessing a struct midway through, with 'i' being
+      // the offset Therefore, if you are accessing a struct, we need to find the
+      // member type. We can go through the members of the struct type and see if
+      // there is a member at the offset you are accessing the struct by. If there
+      // is indeed a member starting at offset 'i' in the struct, we should check
+      // aliasing legality with that type. If there isn't, we run alias checking
+      // on the struct which will give us the correct error.
+      if (OldTD->Tag == TYSAN_STRUCT_TD) {
+        for (int j = 0; j < OldTD->Struct.MemberCount; ++j) {
+          if (OldTD->Struct.Members[j].Offset == i) {
+            AccessedType = OldTD->Struct.Members[j].Type;
+            break;
+          }
+        }
+      }
+    }
 
-    if (!isAliasingLegal(td, OldTD, i))
+    if (!isAliasingLegal(td, AccessedType, i))
       reportError(addr, size, td, OldTD, AccessStr,
                   "accesses part of an existing object", -i, pc, bp, sp);
 
diff --git a/compiler-rt/test/tysan/global-struct-members.c b/compiler-rt/test/tysan/global-struct-members.c
new file mode 100644
index 00000000000000..ed8f21bb84f3c5
--- /dev/null
+++ b/compiler-rt/test/tysan/global-struct-members.c
@@ -0,0 +1,30 @@
+// RUN: %clang_tysan -O0 %s -o %t && %run %t >%t.out 2>&1
+// RUN: FileCheck %s --implicit-check-not ERROR < %t.out
+
+#include <stdio.h>
+
+struct X {
+  int a, b, c;
+} x;
+
+static struct X xArray[2];
+
+int main() {
+  x.a = 1;
+  x.b = 2;
+  x.c = 3;
+
+  printf("%d %d %d\n", x.a, x.b, x.c);
+
+  for (size_t i = 0; i < 2; i++) {
+    xArray[i].a = 1;
+    xArray[i].b = 1;
+    xArray[i].c = 1;
+  }
+
+  struct X *xPtr = (struct X *)&(xArray[0].c);
+  xPtr->a = 1;
+  // CHECK: ERROR: TypeSanitizer: type-aliasing-violation
+  // CHECK-NEXT: WRITE of size 4 at {{.*}} with type int (in X at offset 0) accesses an existing object of type int (in X at offset 8)
+  // CHECK-NEXT: #0 0x{{.*}} in main {{.*}}struct-members.c:[[@LINE-3]]
+}

[github-actions]

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

[gbMattN]

@fhahn Pull request is back up again 👍😅

[gbMattN]

It seems over the development of the original patches this has been fixed. I'll look into it to be sure, but may end up closing this!

[seanm]

I'm seeing some TySan reports that I don't understand, and structs are involved, so I thought I was maybe seeing the same as this ticket, but your original C++ example, below, indeed does not provoke any warning with current master:

#include <stdio.h>
struct S{
    int a, b, c;
};
S s;
int main(){
    s.a = 1;
    s.b = 2;
    s.c = 3;
    printf("%d %d %d\n", s.a, s.b, s.c);
    return 0;
}

[gbMattN]

Does the odd behaviour you're seeing sound like whats described in this pull request?. If not, please open an issue, I'd love to take a look at it!