-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[TSan] Clarify and enforce shadow end alignment #144648
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
Open
Camsyn
wants to merge
8
commits into
llvm:main
Choose a base branch
from
Camsyn:tsan-shadow-end
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+126
−9
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
49f9872
[TSan] Fix potentially problematic shadow end calculations
Camsyn 4c59a82
Fix the wrong description.
Camsyn 0b868ad
Add two test cases related to incorrect shadow-end calculation
Camsyn 89eb222
Use RoundUp on demand instead of a new util function
Camsyn faea01f
add some assertions
Camsyn 1d9f632
Revert "Fix the wrong description."
Camsyn 2f7f865
Make formatter happy
Camsyn bb37bf3
Remove all MemToEndShadow usages
Camsyn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s | ||
#include "java.h" | ||
#include <errno.h> | ||
#include <sys/mman.h> | ||
|
||
int main() { | ||
// Test a non-regular kHeapSize | ||
// Previously __tsan_java_init failed because it encountered non-zero meta | ||
// shadow for the destination. | ||
size_t const kPageSize = sysconf(_SC_PAGESIZE); | ||
int const kSize = kPageSize - 1; | ||
jptr jheap2 = (jptr)mmap(0, kSize, PROT_READ | PROT_WRITE, | ||
MAP_ANON | MAP_PRIVATE, -1, 0); | ||
if (jheap2 == (jptr)MAP_FAILED) | ||
return printf("mmap failed with %d\n", errno); | ||
__atomic_store_n((int *)(jheap2 + kSize - 3), 1, __ATOMIC_RELEASE); | ||
// Due to the previous incorrect meta-end calculation, the following munmap | ||
// did not clear the tail meta shadow. | ||
munmap((void *)jheap2, kSize); | ||
int const kHeapSize2 = kSize + 1; | ||
jheap2 = (jptr)mmap((void *)jheap2, kHeapSize2, PROT_READ | PROT_WRITE, | ||
MAP_ANON | MAP_PRIVATE, -1, 0); | ||
if (jheap2 == (jptr)MAP_FAILED) | ||
return printf("second mmap failed with %d\n", errno); | ||
__tsan_java_init(jheap2, kHeapSize2); | ||
__tsan_java_move(jheap2, jheap2 + kHeapSize2 - 8, 8); | ||
fprintf(stderr, "DONE\n"); | ||
return __tsan_java_fini(); | ||
} | ||
|
||
// CHECK-NOT: WARNING: ThreadSanitizer: data race | ||
// CHECK: DONE |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
// RUN: %clang_tsan %s -o %t && %run %t | FileCheck %s | ||
#include "test.h" | ||
#include <assert.h> | ||
#include <pthread.h> | ||
#include <stdint.h> | ||
#include <stdio.h> | ||
#include <sys/mman.h> | ||
#include <unistd.h> | ||
|
||
void __tsan_read1(void *addr); | ||
|
||
struct thread_params { | ||
char *buf; | ||
unsigned int size; | ||
}; | ||
|
||
static void *thread_func(void *arg) { | ||
struct thread_params *p = (struct thread_params *)arg; | ||
// Access 1 | ||
p->buf[0] = 0x42; | ||
p->buf[p->size - 1] = 0x42; | ||
barrier_wait(&barrier); | ||
return 0; | ||
} | ||
|
||
int main() { | ||
const unsigned int kPageSize = sysconf(_SC_PAGESIZE); | ||
// The relevant shadow memory size should be exactly multiple of kPageSize, | ||
// even if Size = kPageSize - 1. | ||
const unsigned int Size = kPageSize - 1; | ||
|
||
barrier_init(&barrier, 2); | ||
char *buf = (char *)mmap(NULL, Size, PROT_READ | PROT_WRITE, | ||
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); | ||
assert(buf != MAP_FAILED); | ||
assert(((uintptr_t)buf % kPageSize) == 0); | ||
|
||
pthread_t t; | ||
struct thread_params p = {buf, Size}; | ||
pthread_create(&t, 0, thread_func, &p); | ||
|
||
barrier_wait(&barrier); | ||
// Should clear all the shadow memory related to the mmaped memory. | ||
munmap(buf, Size); | ||
|
||
// If the shadow memory is cleared completely, the following read should not | ||
// cause a race. | ||
// CHECK-NOT: WARNING: ThreadSanitizer: data race | ||
__tsan_read1(&buf[0]); // Access 2 | ||
__tsan_read1(&buf[Size - 1]); // Access 2 | ||
pthread_join(t, 0); | ||
|
||
puts("DONE"); | ||
|
||
return 0; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add these test cases to a separate pull request. Since your fix is not committed yet, the tests would need an "XFAIL" annotation. Then, after the test cases are landed, this fix PR would update the test by removing the XFAIL. This two-step procedure makes it clear that 1) TSan was broken 2) this PR fixes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was unfixed in your environment, that was an exception (and would you mind providing more information about the test failure, such as the triggering environment?);
This relevant bug should indeed be fixed by this PR, and I have passed this test (Ubuntu24.04@x86_64) before I pushed the commit.
FYI, this test case manages to trigger a situation that
munmap
clears the meta incompletely due to the incorrect usage of an inclusive meta end. However, this bug should be fixed in this PR, as follows: