Add LOCKREADIFFAILRET to unlocked MDInternalRW read methods#127958
Open
kevingosse wants to merge 2 commits intodotnet:mainfrom
Open
Add LOCKREADIFFAILRET to unlocked MDInternalRW read methods#127958kevingosse wants to merge 2 commits intodotnet:mainfrom
kevingosse wants to merge 2 commits intodotnet:mainfrom
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.
dotnet-policy-service
Bot
added
the
community-contribution
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds read-locking to metadata signature/name accessors and simplifies error handling to return directly instead of using ErrExit/IfFailGo.
Changes:
- Add
LOCKREADIFFAILRET()inGetSigOfMethodDef,GetSigOfFieldDef, andGetNameAndSigOfMemberRef. - In
GetSigFromToken, acquire read locks formdtSignature/mdtTypeSpecand replaceIfFailGo+ErrExitwithIfFailRet+ direct returns. - Simplify invalid-token handling by returning
META_E_INVALID_TOKEN_TYPEdirectly.
Comment on lines
2287
to
2291
| HRESULT hr; | ||
| // We don't change MethodDef signature. No need to lock. | ||
| LOCKREADIFFAILRET(); | ||
|
|
||
| MethodRec *pMethodRec; | ||
| *ppSig = NULL; |
Comment on lines
2312
to
2316
| HRESULT hr; | ||
| // We don't change Field's signature. No need to lock. | ||
| LOCKREADIFFAILRET(); | ||
|
|
||
| FieldRec *pFieldRec; | ||
| *ppSig = NULL; |
| @@ -2332,34 +2332,35 @@ MDInternalRW::GetSigFromToken( | |||
| PCCOR_SIGNATURE * ppSig) | |||
| { | |||
| HRESULT hr; | |||
| @@ -2669,8 +2667,7 @@ MDInternalRW::GetNameAndSigOfMemberRef( // meberref's name | |||
| LPCSTR *pszMemberRefName) | |||
| { | |||
| HRESULT hr; | |||
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
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.
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
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.
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 segmentation faults.
Fix: add LOCKREADIFFAILRET() to GetSigOfMethodDef, GetSigOfFieldDef, GetNameAndSigOfMemberRef, and GetSigFromToken.
Fixes #127957