Skip to content

Add proper locking in TranslateSigHelper - #120143

Merged
jkotas merged 4 commits into
dotnet:mainfrom
kevingosse:metadata_locking_main
Sep 26, 2025
Merged

Add proper locking in TranslateSigHelper#120143
jkotas merged 4 commits into
dotnet:mainfrom
kevingosse:metadata_locking_main

Conversation

@kevingosse

@kevingosse kevingosse commented Sep 26, 2025

Copy link
Copy Markdown
Contributor

TranslateSigHelper updates the metadata without going through the RegMeta interface (by directly extracting the m_pStgdb field). When doing so, it omits to acquire the write lock, which can end up corrupting the metadata when multiple threads are running concurrently.

Fixes #119958

Copilot AI review requested due to automatic review settings September 26, 2025 11:16
@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 26, 2025
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Sep 26, 2025
Copilot AI reviewed Sep 26, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a thread safety issue in the TranslateSigHelper function by adding proper locking when updating metadata. The function was directly accessing and modifying metadata without acquiring the necessary write lock, which could lead to data corruption in multi-threaded scenarios.

Key Changes

  • Add proper write lock acquisition before metadata operations
  • Restructure function to use error handling pattern with goto-based cleanup
  • Ensure thread-safe access to metadata structures

Comment thread src/coreclr/md/enc/mdinternalrw.cpp Outdated
@kevingosse kevingosse mentioned this pull request Sep 26, 2025
Closed
@build-analysis build-analysis Bot mentioned this pull request Sep 26, 2025
Closed
@jkotas jkotas added area-TypeSystem-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 26, 2025
@kevingosse

kevingosse commented Sep 26, 2025

Copy link
Copy Markdown
Contributor Author 
  /__w/1/s/src/coreclr/md/enc/mdinternalrw.cpp:63:5: error: cannot jump from this goto statement to its label
     63 |     IfFailGo(cSem.LockWrite());
        |     ^
  /__w/1/s/src/coreclr/inc/debugmacros.h:157:24: note: expanded from macro 'IfFailGo'
    157 | #define IfFailGo(EXPR) IfFailGoto(EXPR, ErrExit)
        |                        ^
  /__w/1/s/src/coreclr/inc/debugmacros.h:146:36: note: expanded from macro 'IfFailGoto'
    146 | do { hr = (EXPR); if(FAILED(hr)) { goto LABEL; } } while (0)
        |                                    ^
  /__w/1/s/src/coreclr/md/enc/mdinternalrw.cpp:65:23: note: jump bypasses variable initialization
     65 |     IMetaModelCommon *pCommonAssemImport = pAssemImport ? pAssemImport->GetMetaModelCommon() : NULL;
        |                       ^
  1 error generated.

I don't have this error when building locally. I guess MSVC has different rules? Anyway, I pushed a tentative fix. Unfortunately I don't have a Linux environment ready to test.

jkotas
jkotas approved these changes Sep 26, 2025
View reviewed changes

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

jkotas
jkotas reviewed Sep 26, 2025
View reviewed changes
Comment thread src/coreclr/md/enc/mdinternalrw.cpp Outdated
@jkotas
jkotas merged commit 0b520b8 into dotnet:main Sep 26, 2025
96 checks passed
@jkotas

jkotas commented Sep 26, 2025

Copy link
Copy Markdown
Member

/backport to release/10.0

@github-actions

github-actions Bot commented Sep 26, 2025

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/18049188130

@github-actions github-actions Bot mentioned this pull request Sep 26, 2025
Merged
2 of 4 tasks
@kevingosse
kevingosse deleted the metadata_locking_main branch September 26, 2025 21:16
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Sep 26, 2025

Copy link
Copy Markdown
Member

/cc @jkoritzinsky

@github-actions github-actions Bot locked and limited conversation to collaborators Oct 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

area-TypeSystem-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CMiniMdRW::PutString stuck in infinite loop

4 participants

@kevingosse @jkotas @AaronRobinsonMSFT