Add LOCKREADIFFAILRET to unlocked MDInternalRW read methods#127958
Add LOCKREADIFFAILRET to unlocked MDInternalRW read methods#127958kevingosse 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
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.
| HRESULT hr; | ||
| // We don't change MethodDef signature. No need to lock. | ||
| LOCKREADIFFAILRET(); | ||
|
|
||
| MethodRec *pMethodRec; | ||
| *ppSig = NULL; |
| 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; | |||
|
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.
|
Thanks @kevingosse - this is a nice find!
How did you land on this list methods? I ask because I am wondering if there are more methods also affected by the missing lock. For example is this one another case of the same missing lock problem? @agocke - do we have a dev owner for the metadata code these days? I know it doesn't change often and I'm happy to review but if we do have an owner I wanted them to be in the loop on this too. |
|
You're right, I focused on the set of methods that appeared in the crashes but it looks like there are a bunch more with the same pattern.
|
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