Skip to content

Fix race condition in DacEnumerableHashTable::BaseFindNextEntryByHash - #75099

Merged
AntonLapounov merged 3 commits into
dotnet:mainfrom
AntonLapounov:FixRaceInBaseFindNextEntryByHash
Sep 7, 2022
Merged

Fix race condition in DacEnumerableHashTable::BaseFindNextEntryByHash#75099
AntonLapounov merged 3 commits into
dotnet:mainfrom
AntonLapounov:FixRaceInBaseFindNextEntryByHash

Conversation

@AntonLapounov

Copy link
Copy Markdown
Contributor

See #75041 (comment) for details. Fixes#75041.

@ghostghost added the area-VM-coreclr label Sep 6, 2022
Comment threadsrc/coreclr/vm/dacenumerablehash.inl Outdated
@jkotas
jkotas requested a review from VSadovSeptember 6, 2022 03:07
@VSadov

Copy link
Copy Markdown
Member

I think VolatileEntry needs to be renamed just an Entry. The name seems deceiving.

@VSadov

Copy link
Copy Markdown
Member

Or perhaps m_pNextEntry should be declared as volatile - in "ISO volatile" sense. I generally prefer explicit VolatileLoadWithoutBarrier as more obvious at the point of reading, but if all the places where we read m_pNextEntry need to be changed and there are lots of them, maybe volatile would work better.
Perhaps that is what was meant when the type got "Volatile" in its name and we lost it somehow.

@AntonLapounov

Copy link
Copy Markdown
ContributorAuthor

The failure in the 'Build OSX x64 release NativeAOT' job is #75005.

@AntonLapounov

Copy link
Copy Markdown
ContributorAuthor

but if all the places where we read m_pNextEntry need to be changed and there are lots of them, maybe volatile would work better.

Volatile loads are not necessary on the writer code path. I tried to make a safest/easiest fix for porting to 7.0. Do the changes look reasonable?

@AntonLapounov
AntonLapounov merged commit a7eda3e into dotnet:mainSep 7, 2022
@AntonLapounov
AntonLapounov deleted the FixRaceInBaseFindNextEntryByHash branch September 7, 2022 03:19
@AntonLapounov

Copy link
Copy Markdown
ContributorAuthor

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3004820401

@ghostghost locked as resolved and limited conversation to collaborators Oct 7, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in DacEnumerableHashTable::BaseFindNextEntryByHash

3 participants

@AntonLapounov@VSadov@jkotas