Fix ASan handling of WasmOffsetConverter reading on a pthread #14611
Conversation
This file contains 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
sbc100
reviewed
Jul 26, 2021
| 'Thread T1 created by T0 here:', | ||
| 'SUMMARY: AddressSanitizer: heap-use-after-free', | ||
| 'Shadow bytes around the buggy address:', | ||
| 'Shadow byte legend (one shadow byte represents 8 application bytes):', |
There was a problem hiding this comment.
When these tests fail its really hard to know why.. I wonder if we can pipe the stderr back to the server and use the same output comparison methods we use in non-browser tests?
Do you know how common this pattern is in browser tests? Is it just the sanitizer tests?
There was a problem hiding this comment.
I think I've only seen this pattern in the sanitizer tests. Yeah, I agree it's a little inconvenient, +1 for looking into possible improvements.
sbc100
added a commit
that referenced
this pull request
Jul 27, 2021
Following up on the change made in #14611, this change forces the `WasmOffsetConverter` before calling receiveInstantiationResult in both code paths (instantiateArrayBuffer and instantiateAsync). With that done (along with #14755) there are no more valid use cases for to block executation based on the absence of `WasmOffsetConverter`. By the time the instance is recieved it is always defined.
sbc100
added a commit
that referenced
this pull request
Jul 27, 2021
Following up on the change made in #14611, this change forces the `WasmOffsetConverter` before calling receiveInstantiationResult in both code paths (instantiateArrayBuffer and instantiateAsync). With that done (along with #14755) there are no more valid use cases for to block execution based on the absence of `WasmOffsetConverter`. By the time the instance is received it is always defined.
sbc100
added a commit
that referenced
this pull request
Jul 27, 2021
Following up on the change made in #14611, this change forces the `WasmOffsetConverter` before calling receiveInstantiationResult in both code paths (instantiateArrayBuffer and instantiateAsync). With that done (along with #14755) there are no more valid use cases for to block execution based on the absence of `WasmOffsetConverter`. By the time the instance is received it is always defined.
sbc100
added a commit
that referenced
this pull request
Jul 27, 2021
Following up on the change made in #14611, this change forces the `WasmOffsetConverter` before calling receiveInstantiationResult in both code paths (instantiateArrayBuffer and instantiateAsync). With that done (along with #14755) there are no more valid use cases for to block execution based on the absence of `WasmOffsetConverter`. By the time the instance is received it is always defined.
sbc100
added a commit
that referenced
this pull request
Jul 27, 2021
Following up on the change made in #14611, this change forces the `WasmOffsetConverter` before calling receiveInstantiationResult in both code paths (instantiateArrayBuffer and instantiateAsync). With that done (along with #14755) there are no more valid use cases for to block execution based on the absence of `WasmOffsetConverter`. By the time the instance is received it is always defined.
sbc100
added a commit
that referenced
this pull request
Jul 27, 2021
Following up on the change made in #14611, this change forces the `WasmOffsetConverter` before calling receiveInstantiationResult in both code paths (instantiateArrayBuffer and instantiateAsync). With that done (along with #14755) there are no more valid use cases for to block execution based on the absence of `WasmOffsetConverter`. By the time the instance is received it is always defined.
sbc100
added a commit
that referenced
this pull request
Jul 27, 2021
Following up on the change made in #14611, this change forces the `WasmOffsetConverter` before calling `receiveInstantiationResult` in both code paths (instantiateArrayBuffer and instantiateAsync). With that done (along with #14755) there are no more valid use cases for to block execution based on the absence of `WasmOffsetConverter`. By the time the instance is received it is always defined.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
If ASan wants to show a stack trace on a pthread (like the trace to get to
an allocation that was later used after free), then it needs access to the
WasmOffsetConverter. We had a race there, where it could postMessage
the WasmOffsetConverter before that object was actually filled with the
data it needs (function offsets). See details in comments.
Fixes #13205