Skip to content

[5.10🍒] autoreleasing isolated foreign errors #70546

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

kavon
Copy link
Member

@kavon kavon commented Dec 19, 2023

  • Explanation: An @autoreleasing value returned from an async function is not being retained prior to changing executors/actors, only afterwards. If the caller and callee have different actors, then an executor hop will happen, which triggers the autorelease pool and thus destroying the returned value. Upon returning from the hop, we will attempt to retain the destroyed value and trigger a use-after-free / crash in the runtime due to incrementing a ref count of a destroyed object. Here's a visual of the change:
// Bad, prior situation:
let autoreleasingResult = await call(...)
hop_to_executor caller_actor
retain autoreleasingResult

// Corrected:
let autoreleasingResult = await call(...)
retain autoreleasingResult
hop_to_executor caller_actor
  • Radars/Issues: rdar://114049646
  • Scope: Limited to just SILGen and basically just delays the emission of the hop-back until immediately after we retrieve the results and perform clean-ups on arguments after an async call.
  • Risk: Medium. It's a change to SILGen that will slightly perturb the SIL for many async calls (namely, an end_borrow that normally is right after the hop now comes before it), though in most cases it should lead to no changes in the final assembly.
  • Testing: regression test included, in addition to manual testing.
    Reviewed By: @gottesmm
  • main PR: [SILGen] avoid hop before autoreleased foreign error is retained #68415

@kavon kavon requested a review from a team as a code owner December 19, 2023 20:41
@kavon kavon requested review from jckarter and gottesmm December 19, 2023 20:54
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

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

LGTM.

@kavon kavon force-pushed the 5.10-autoreleasing-isolated-foreign-errors branch from 90a514a to c814123 Compare December 19, 2023 21:22
For an isolated ObjC function that is not async, we
emit a hops around the call. But if that function
returns an autoreleased pointer, we need to ensure
we're retaining that pointer before hopping back
after the call. We weren't doing that in the case
of an autoreleased NSError:

```
%10 = alloc_stack $@sil_unmanaged Optional<NSError>
%19 = ... a bunch of steps to wrap up %10 ...
%20 = enum $Optional<AutoreleasingUnsafeMutablePointer<Optional<NSError>>>, #Optional.some!enumelt, %19 : $AutoreleasingUnsafeMutablePointer<Optional<NSError>>
hop_to_executor $MainActor
%26 = apply X(Y, %20) : $@convention(objc_method) (NSObject, Optional<AutoreleasingUnsafeMutablePointer<Optional<NSError>>>) -> @autoreleased Optional<NSString>
hop_to_executor $Optional<Builtin.Executor>
// retain the autoreleased pointer written-out.
%28 = load [trivial] %10 : $*@sil_unmanaged Optional<NSError>
%29 = unmanaged_to_ref %28 : $@sil_unmanaged Optional<NSError> to $Optional<NSError>
%30 = copy_value %29 : $Optional<NSError>
assign %31 to %7 : $*Optional<NSError>
```

This patch sinks the hop emission after the call
so it happens after doing that copy.

rdar://114049646
(cherry picked from commit 06ac850)
@kavon kavon force-pushed the 5.10-autoreleasing-isolated-foreign-errors branch from c814123 to 6658c38 Compare December 19, 2023 21:51
@kavon
Copy link
Member Author

kavon commented Dec 19, 2023

@swift-ci please test

@kavon kavon enabled auto-merge December 20, 2023 01:27
@kavon kavon merged commit de10d01 into swiftlang:release/5.10 Dec 20, 2023
@kavon kavon deleted the 5.10-autoreleasing-isolated-foreign-errors branch December 20, 2023 21:42
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.

3 participants