-
Notifications
You must be signed in to change notification settings - Fork 225
wip: sanitizers #2575
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
base: master
Are you sure you want to change the base?
wip: sanitizers #2575
Conversation
PR HealthChangelog Entry ❗
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
739b45e to
c10e70a
Compare
71c3dbd to
123dfdb
Compare
natebosch
left a comment
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.
Can you edit the first comment on the PR to add some details about this change and the motivation? We can use the first comment to decide the content of the final commit message when this lands.
| test('uninitialized', () { | ||
| var a = malloc(8); | ||
| var b = malloc(8); | ||
| memcmp(a, b, 8); | ||
| free(b); | ||
| free(a); | ||
| }); |
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 the test is intentionally testing only that some code runs without exception we should make that explicit.
| test('uninitialized', () { | |
| var a = malloc(8); | |
| var b = malloc(8); | |
| memcmp(a, b, 8); | |
| free(b); | |
| free(a); | |
| }); | |
| test('uninitialized', () { | |
| expect(() { | |
| var a = malloc(8); | |
| var b = malloc(8); | |
| memcmp(a, b, 8); | |
| free(b); | |
| free(a); | |
| }, returnsNormally); | |
| }); |
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.
No, this code is testing MSAN flagging use of uninitialized memory.
| } | ||
|
|
||
| void main() { | ||
| test('data race', () async { |
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.
Similar to the above, we should match against returnsNormally to indicate blocks that we expect only to run without exceptions.
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.
is this unintentionally empty?
| process.exitCode.then((exitCode) async { | ||
| if (exitCode != 0) { | ||
| // At least don't hang. | ||
| exit(exitCode); |
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.
We have had difficulty in the past using a direct exit call from the test runner, I don't think we should land another.
What are the scenarios where this would hang?
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.
When the sanitizers exit the child, the parent was hanging. This is hacky, and probably the real issue is the consumer of the socket created between parent and child is failing to react to a disconnect, but I haven't found that code yet.
| socket.address.host, | ||
| socket.port.toString(), | ||
| ]); | ||
| ], mode: ProcessStartMode.inheritStdio); |
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.
Why do we need to change the start mode?
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.
This is a better version of the removed stdout/stderr forwarding. In particular, when the parent has ttys instead of pipes, the child will have the same ttys and the sanitizer in the child will emit colored output. It also avoids issues with the parent exiting before all the output is forwarded.
| Future<String> _compileToNative(String path, Metadata suiteMetadata) async { | ||
| /// Compiles [path] to a native shared library using `dart compile aot-snapshot`. | ||
| Future<String> _compileToNative( | ||
| SuitePlatform platform, |
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.
I am unsure about modeling this as a runtime when the only effect is has is to change compiler arguments. What do you think about modeling this as a different compiler?
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.
Using the sanitizers involves changes to both the compiler and the runtime. It's more like running a different program than running the same program with a different strategy. In the SDK's test harness, we model the sanitizers as a separate dimension alongside architecture, compiler and runtime.
123dfdb to
f7b2b78
Compare
5672c15 to
ebf452d
Compare
ebf452d to
879394d
Compare
Adds the ability to run tests on the standalone Dart VM under Address Sanitizer, Memory Sanitizer or Thread Sanitizer.
This is useful for finding issues when using foreign libraries through dart:ffi, such as use-after-free, use of initialized memory and data races.
This is also useful for pure Dart programs that might have data races through the use of shared fields.
Only available on 64-bit Linux.
https://clang.llvm.org/docs/AddressSanitizer.html
https://clang.llvm.org/docs/MemorySanitizer.html
https://clang.llvm.org/docs/ThreadSanitizer.html