[TySan] Fixed false positive when accessing offset member variables #95387

gbMattN · 2024-06-13T10:45:46Z

This patch fixes a bug the current TySan implementation has. Currently if you access a member variable other than the first, TySan reports an error. TySan believes you are accessing the struct type with an offset equal to the offset of the member variable you are trying to access.
With this patch, the type we are trying to access is amended to the type of the member variable matching the offset we are accessing with. It does this if and only if there is a member at that offset, however, so any incorrect accesses are still caught. This is checked in the struct-members.c test.

github-actions · 2024-06-13T10:46:04Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-13T10:46:34Z

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

Author: None (gbMattN)

Changes

This patch fixes a bug the current TySan implementation has. Currently if you access a member variable other than the first, TySan reports an error. TySan believes you are accessing the struct type with an offset equal to the offset of the member variable you are trying to access.
With this patch, the type we are trying to access is amended to the type of the member variable matching the offset we are accessing with. It does this if and only if there is a member at that offset, however, so any incorrect accesses are still caught. This is checked in the struct-members.c test.

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

2 Files Affected:

(modified) compiler-rt/lib/tysan/tysan.cpp (+11-1)
(added) compiler-rt/test/tysan/struct-members.c (+32)

diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index f627851d049e6..747727e48a152 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -221,7 +221,17 @@ __tysan_check(void *addr, int size, tysan_type_descriptor *td, int flags) {
     OldTDPtr -= i;
     OldTD = *OldTDPtr;
 
-    if (!isAliasingLegal(td, OldTD))
+    tysan_type_descriptor *InternalMember = OldTD;
+    if (OldTD->Tag == TYSAN_STRUCT_TD) {
+      for (int j = 0; j < OldTD->Struct.MemberCount; j++) {
+        if (OldTD->Struct.Members[j].Offset == i) {
+          InternalMember = OldTD->Struct.Members[j].Type;
+          break;
+        }
+      }
+    }
+
+    if (!isAliasingLegal(td, InternalMember))
       reportError(addr, size, td, OldTD, AccessStr,
                   "accesses part of an existing object", -i, pc, bp, sp);
 
diff --git a/compiler-rt/test/tysan/struct-members.c b/compiler-rt/test/tysan/struct-members.c
new file mode 100644
index 0000000000000..8cf6499f78ce6
--- /dev/null
+++ b/compiler-rt/test/tysan/struct-members.c
@@ -0,0 +1,32 @@
+// RUN: %clang_tysan -O0 %s -o %t && %run %t >%t.out 2>&1
+// RUN: FileCheck %s < %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);
+  // CHECK-NOT: ERROR: TypeSanitizer: type-aliasing-violation
+
+  for (size_t i = 0; i < 2; i++) {
+    xArray[i].a = 1;
+    xArray[i].b = 1;
+    xArray[i].c = 1;
+  }
+  printf("Here\n");
+
+  struct X *xPtr = (struct X *)&(xArray[0].c);
+  xPtr->a = 1;
+  // CHECK: ERROR: TypeSanitizer: type-aliasing-violation
+  // CHECK: 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: {{#0 0x.* in main .*struct-members.c:}}[[@LINE-3]]
+}

gbMattN · 2024-06-13T13:20:52Z

@fhahn

gbMattN · 2024-06-13T16:32:52Z

This may be a side effect of a different bug tracking global variables. I think fixing that bug first, and then applying this change if the problem persists is a better idea. Because of this, I'm switching this to a draft for now. Discourse link is https://discourse.llvm.org/t/reviving-typesanitizer-a-sanitizer-to-catch-type-based-aliasing-violations/66092/23

fhahn · 2024-06-27T14:38:29Z

compiler-rt/lib/tysan/tysan.cpp

@@ -221,7 +221,17 @@ __tysan_check(void *addr, int size, tysan_type_descriptor *td, int flags) {
    OldTDPtr -= i;
    OldTD = *OldTDPtr;

-    if (!isAliasingLegal(td, OldTD))
+    tysan_type_descriptor *InternalMember = OldTD;


Could you add a comment here indicating what this does?

Done! Let me know if I need to clarify anything in it.

I feel like you should name the variable declared here "AccessedType" or similar, as the default-path is to check access to OldTD rather than an internal member of it, right?

jmorse

I'm not familiar with TySan, but here's some style feedback and similar inline.

The comments in tysan.cpp are too long (clang-format-bot would have bitten you if it was live when you uploaded this), please clang-format.

jmorse · 2024-11-11T17:40:52Z

compiler-rt/lib/tysan/tysan.cpp

+    // 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 with will give us the correct error.


Suggested change

// with that type. If there isn't, we run alias checking on the struct with will give us the correct error.

// with that type. If there isn't, we run alias checking on the struct which will give us the correct error.

Doesn't parse without this change?

jmorse · 2024-11-11T17:42:04Z

compiler-rt/lib/tysan/tysan.cpp

+    // with that type. If there isn't, we run alias checking on the struct with will give us the correct error.
+    tysan_type_descriptor *InternalMember = OldTD;
+    if (OldTD->Tag == TYSAN_STRUCT_TD) {
+      for (int j = 0; j < OldTD->Struct.MemberCount; j++) {


Style guide says ++j

jmorse · 2024-11-11T17:43:52Z

compiler-rt/lib/tysan/tysan.cpp

@@ -221,7 +221,17 @@ __tysan_check(void *addr, int size, tysan_type_descriptor *td, int flags) {
    OldTDPtr -= i;
    OldTD = *OldTDPtr;

-    if (!isAliasingLegal(td, OldTD))
+    tysan_type_descriptor *InternalMember = OldTD;


I feel like you should name the variable declared here "AccessedType" or similar, as the default-path is to check access to OldTD rather than an internal member of it, right?

jmorse · 2024-11-11T17:47:11Z

compiler-rt/test/tysan/global-struct-members.c

+  x.c = 3;
+
+  printf("%d %d %d\n", x.a, x.b, x.c);
+  // CHECK-NOT: ERROR: TypeSanitizer: type-aliasing-violation


I'm not familiar with the output of TySan, but this CHECK-NOT paired with the CHECK on line 28 is going to test that "there's no error until there's an error", which makes this specific CHECK-NOT redundant. If you're looking to ensure there's only one instance of that error-output then you need to put another CHECK-NOT under the CHECK lines below, or add --implicit-check-not to FileCheck. Also consider just not having this CHECK-NOT line, if TySan will never produce more than one error.

jmorse · 2024-11-11T17:48:59Z

compiler-rt/test/tysan/global-struct-members.c

+  // CHECK: ERROR: TypeSanitizer: type-aliasing-violation
+  // CHECK: 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: {{#0 0x.* in main .*struct-members.c:}}[[@LINE-3]]


Is it possible to strengthen this test even further by making the latter two "CHECK-NEXT", to ensure the error appears all in one block? (Reduces the risk of spurious extra output matching CHECK lines).

I'd recommend reducing the portions of the CHECK-line that are regexes to be as small as possible, so {{.*}}struct-members.c and similar.

Created using spr 1.3.4

…riables

…he first time

github-actions · 2024-12-02T17:20:53Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 8e6e62d0dee48a696afd0c7d53d74eaccef97b5e 8addd061964253a1100d76446569ff1f67e63a9c --extensions cpp,c -- compiler-rt/test/tysan/global-struct-members.c compiler-rt/lib/tysan/tysan.cpp

View the diff from clang-format here.

diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index 228a5f13f5..da92e64181 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -226,20 +226,21 @@ __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 != 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) {

fhahn · 2024-12-17T18:55:42Z

Sorry I didn't mean to close this PR, looks like it happened automatically once I deleted the users/fhahn/tysan-a-type-sanitizer-runtime-library branch :(

llvmbot added compiler-rt compiler-rt:sanitizer labels Jun 13, 2024

gbMattN force-pushed the users/nagym/tysan-struct-member-fix branch from f7e775f to 432f994 Compare June 13, 2024 11:15

gbMattN marked this pull request as draft June 13, 2024 16:33

gbMattN marked this pull request as ready for review June 27, 2024 12:27

fhahn reviewed Jun 27, 2024

View reviewed changes

gbMattN force-pushed the users/nagym/tysan-struct-member-fix branch from 432f994 to 8b9530d Compare June 27, 2024 14:54

fhahn force-pushed the users/fhahn/tysan-a-type-sanitizer-runtime-library branch from e2caf4e to 733b3ed Compare June 27, 2024 16:10

gbMattN force-pushed the users/nagym/tysan-struct-member-fix branch from 8b9530d to 547444a Compare June 28, 2024 16:32

gbMattN force-pushed the users/nagym/tysan-struct-member-fix branch from 547444a to 8099113 Compare July 12, 2024 15:31

gbMattN requested a review from fhahn July 12, 2024 16:21

gbMattN force-pushed the users/nagym/tysan-struct-member-fix branch from 83a3688 to 8099113 Compare September 12, 2024 10:54

jmorse reviewed Nov 11, 2024

View reviewed changes

gbMattN requested a review from jmorse November 12, 2024 17:06

fhahn added 6 commits November 22, 2024 14:19

[𝘀𝗽𝗿] initial version

be460ed

Created using spr 1.3.4

!fixup address comments, thanks

9a6e027

!fiupx address comments, thanks!

a6d44dd

[TySan] A Type Sanitizer (Clang)

ab8d005

!fixup: add test

9cbb116

[TySan] A Type Sanitizer (Runtime Library)

8e6e62d

fhahn force-pushed the users/fhahn/tysan-a-type-sanitizer-runtime-library branch from 733b3ed to 8e6e62d Compare November 22, 2024 19:34

Matthew Nagy and others added 2 commits November 29, 2024 11:51

[TySan] Fixed false positive when accessing global object's member va…

620ee82

…riables

[style] Tidied up comments, formatting, and polished test

9a2c70c

gbMattN force-pushed the users/nagym/tysan-struct-member-fix branch from be4f4c5 to 9a2c70c Compare November 29, 2024 11:52

Removed false positive/segfault when accessing member of global for t…

8addd06

…he first time

fhahn force-pushed the users/fhahn/tysan-a-type-sanitizer-runtime-library branch 3 times, most recently from 5c0e520 to 6f48a28 Compare December 10, 2024 12:22

fhahn deleted the branch llvm:users/fhahn/tysan-a-type-sanitizer-runtime-library December 17, 2024 18:49

fhahn closed this Dec 17, 2024

gbMattN mentioned this pull request Dec 18, 2024

[TySan] Fixed false positive when accessing offset member variables #120406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TySan] Fixed false positive when accessing offset member variables #95387

[TySan] Fixed false positive when accessing offset member variables #95387

Uh oh!

gbMattN commented Jun 13, 2024

Uh oh!

github-actions bot commented Jun 13, 2024

Uh oh!

llvmbot commented Jun 13, 2024

Uh oh!

gbMattN commented Jun 13, 2024

Uh oh!

gbMattN commented Jun 13, 2024

Uh oh!

fhahn Jun 27, 2024

Uh oh!

gbMattN Jun 27, 2024 •

edited

Loading

Uh oh!

jmorse Nov 11, 2024

Uh oh!

jmorse left a comment

Uh oh!

jmorse Nov 11, 2024

Uh oh!

jmorse Nov 11, 2024

Uh oh!

jmorse Nov 11, 2024

Uh oh!

jmorse Nov 11, 2024

Uh oh!

jmorse Nov 11, 2024

Uh oh!

github-actions bot commented Dec 2, 2024

Uh oh!

fhahn commented Dec 17, 2024

Uh oh!

Uh oh!

	// with that type. If there isn't, we run alias checking on the struct with will give us the correct error.
	// with that type. If there isn't, we run alias checking on the struct which will give us the correct error.

[TySan] Fixed false positive when accessing offset member variables #95387

[TySan] Fixed false positive when accessing offset member variables #95387

Uh oh!

Conversation

gbMattN commented Jun 13, 2024

Uh oh!

github-actions bot commented Jun 13, 2024

Uh oh!

llvmbot commented Jun 13, 2024

Uh oh!

gbMattN commented Jun 13, 2024

Uh oh!

gbMattN commented Jun 13, 2024

Uh oh!

fhahn Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

gbMattN Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmorse Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

jmorse left a comment

Choose a reason for hiding this comment

Uh oh!

jmorse Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

jmorse Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

jmorse Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

jmorse Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

jmorse Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 2, 2024

Uh oh!

fhahn commented Dec 17, 2024

Uh oh!

Uh oh!

gbMattN Jun 27, 2024 •

edited

Loading