Skip to content

[5.4][Runtime] Fix incorrect memory ordering in ConcurrentReadableArray/HashMap #35369

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.

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

Merged

Conversation

mikeash
Copy link
Contributor

@mikeash mikeash commented Jan 12, 2021

Cherry-pick #35348 and #35383 to 5.4.

When reallocating storage, the storing the pointer to the new storage had insufficiently strong ordering. This could cause writers to check the reader count before storing the new storage pointer. If a reader then came in between that load and store, it would end up using the old storage pointer, while the writer could end up freeing it.

Also adjust the Concurrent.cpp tests to test with a variety of reader and writer counts. Counterintuitively, when freeing garbage is gated on there being zero readers, having a single reader will shake out problems that having lots of readers will not. We were testing with lots of readers in order to stress the code as much as possible, but this resulted in it being extremely rare for writers to ever see zero active readers.

rdar://69798617

@mikeash mikeash requested review from davezarzycki and tbkka January 12, 2021 18:11
@mikeash mikeash requested a review from a team as a code owner January 12, 2021 18:11
@mikeash mikeash changed the title [Runtime] Fix incorrect memory ordering in ConcurrentReadableArray/HashMap [5.4][Runtime] Fix incorrect memory ordering in ConcurrentReadableArray/HashMap Jan 12, 2021
@mikeash
Copy link
Contributor Author

mikeash commented Jan 12, 2021

@swift-ci please test

Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

…shMap.

When reallocating storage, the storing the pointer to the new storage had insufficiently strong ordering. This could cause writers to check the reader count before storing the new storage pointer. If a reader then came in between that load and store, it would end up using the old storage pointer, while the writer could end up freeing it.

Also adjust the Concurrent.cpp tests to test with a variety of reader and writer counts. Counterintuitively, when freeing garbage is gated on there being zero readers, having a single reader will shake out problems that having lots of readers will not. We were testing with lots of readers in order to stress the code as much as possible, but this resulted in it being extremely rare for writers to ever see zero active readers.

rdar://69798617
(cherry picked from commit ef8d20a)
@mikeash mikeash force-pushed the fix-concurrentarray-memory-order2-5.4 branch from 3f65e6f to 118a6b0 Compare January 12, 2021 22:44
@mikeash
Copy link
Contributor Author

mikeash commented Jan 12, 2021

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 118a6b0

@mikeash
Copy link
Contributor Author

mikeash commented Jan 13, 2021

@swift-ci please test os x platform

@mikeash mikeash merged commit bd89e61 into swiftlang:release/5.4 Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants