[AST] Treat actors inheriting from NSObject as SwiftNativeNSObjects. #38803
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, there was a divergence in behavior between marking an actor as: - objc: This would lead to usage of Swift reference counting. - subclass of NSObject but not objc: This would lead to usage of ObjC reference counting. By itself, this might seem OK, after all, the two reference counting schemes are compatible (you can pass things from one language to another). However, there is a difference here because actors have a multi-step deinitialization process due to the way the reference accounting works with tasks. When the last reference is released, the destroy() method is called which only puts the actor into a Zombie_Latching state and deallocation happens later on. In contrast, when the ObjC runtime releases the last reference, it immediately deallocates the actor, without calling the destroy() method or having any awareness about actors. This mismatch leads to a use-after-free in the Swift runtime; normally, the actor would be in the Zombie_Latching state and when the final running task finishes, it would end up deallocating the actor. This expectation is not met when the memory is freed by the ObjC runtime, hence the UAF. Normally, I would find it a bit odd to see a commit come with acknowledgements but in this case I think maybe it's okay, given that this took me 2+ weeks to investigate and fix. Thanks to: - Dani C. in helping reproduce the issue with a large and small test case. - John for answering questions about the actor and job lifecycle. - Mike Ash for providing generous help with debugging. Fixes rdar://80863853.
|
I wasn't entirely sure if the best place to test this was at the SIL level, or LLVM IR, or if I should add a runtime test too to make sure the superclass is indeed |
|
@swift-ci smoke test |
DougGregor
approved these changes
Aug 9, 2021
|
LGTM; this should have been kept in the condition when Doug introduced the |
Seems like it should be an assertion that any default actor uses Swift reference counting |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, omitting
@objcmeant that ObjC-refcounting was used (even if: NSObjectwas present), meaning that the multi-step deinitialization of different zombie states was
bypassed, leading to eager deallocation on the last release and user-after-free.
Fixes rdar://80863853.