Skip to content

Add proper locking in TranslateSigHelper #120143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jkotas merged 4 commits into from
Sep 26, 2025
Merged

Add proper locking in TranslateSigHelper #120143

jkotas merged 4 commits into from
Sep 26, 2025

Conversation

@kevingosse
Copy link
Contributor

@kevingosse kevingosse commented Sep 26, 2025

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
Copy link
Contributor

Copilot AI left a comment

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

@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
Copy link
Contributor Author

kevingosse commented Sep 26, 2025 

  /__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
Copy link
Member

@jkotas jkotas left a comment

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
@jkotas jkotas merged commit 0b520b8 into dotnet:main Sep 26, 2025
96 checks passed
@jkotas
Copy link
Member

jkotas commented Sep 26, 2025

/backport to release/10.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2025

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
4 tasks
@kevingosse kevingosse deleted the metadata_locking_main branch September 26, 2025 21:16
@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Sep 26, 2025

/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 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

3 participants

@kevingosse @jkotas @AaronRobinsonMSFT