[release/8.0-staging] Add LOCKREADIFFAILRET to unlocked MDInternalRW read methods#128733
Closed
github-actions[bot] wants to merge 4 commits into
Closed
[release/8.0-staging] Add LOCKREADIFFAILRET to unlocked MDInternalRW read methods#128733github-actions[bot] wants to merge 4 commits into
github-actions[bot] wants to merge 4 commits into
Conversation
Several read methods in MDInternalRW skip locking because they assume row contents are never modified. However, ExpandTables() reallocates the backing buffer of every metadata table, invalidating pointers returned by GetMethodRecord/GetFieldRecord/GetMemberRefRecord. This races with profiler emit operations (DefineMemberRef, etc.) that trigger ExpandTables under LOCKWRITE, causing use-after-free reads that manifest as sporadic MissingMethodException or access violations. Fix: add LOCKREADIFFAILRET() to GetSigOfMethodDef, GetSigOfFieldDef, GetNameAndSigOfMemberRef, and GetSigFromToken.
Address review feedback: if LOCKREADIFFAILRET fails, callers should still see deterministic out-param values (NULL/0) rather than uninitialized memory. Move out-param initialization above the lock.
Same race as the prior commits: read methods skip locking with "no need to lock" comments, but ExpandTables() reallocates the backing buffer of every metadata table under LOCKWRITE, invalidating record pointers from Get*Record(). Fix the remaining methods that dereference record pointers without holding the read lock: - GetCustomAttributeProps - GetCustomAttributeAsBlob - GetNameOfTypeDef - GetNameOfMethodDef - GetNameOfFieldDef - GetNameOfTypeRef - GetTypeOfInterfaceImpl - GetMethodSpecProps - GetTypeSpecFromToken Out-params are initialized before the lock so callers see deterministic NULL/0 if LockRead fails.
Contributor
|
Tagging subscribers to this area: @agocke |
Member
|
.NET 8 is in maintenance support, which is security fixes only. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Backport of #127958 to release/8.0-staging
/cc @elinor-fung @kevingosse
Customer Impact
Issue: #127957
Some read operations for metadata tables were not done under a lock. When the tables are expanded - for example via profiler emit operations - concurrent reads can end up with invalid pointers resulting in a segfault or a MissingMethodException.
DataDog has been hitting it multiple times a week in CI and has gotten customer reports as well.
Regression
Testing
@andrewlock validated against CI that was hitting this issue without this fix: #127958 (comment)
@kevingosse validated against local changes that were reproing the issue before his fix: #127958 (comment)
Risk
Low.