Implicit escaping of closures via Objective-C


(Douglas Gregor) #1

We recently discovered an issue where non-escaping closure parameters (the default since Swift 3) can escape through Objective-C without the user realizing that they’ve done something wrong. The easiest way to see the problem is to define an @objc protocol with a closure-taking method:

@objc protocol Fooable {
  func foo(completion: () -> Void)  // note:closure  parameter is implicitly '@noescape'
}

func useFooable(fooable: Fooable, completion: () -> Void) {
  fooable.foo(completion: completion)
}

And then implement that protocol in Objective-C

@interface MyClass : NSObject <Fooable>
@end

@implementation MyClass
- (void)fooWithCompletion:(void (^)(void))completion {
  dispatch_async(dispatch_get_main_queue(), completion);  // oops, we escaped a no-escape block!
}
@end

One could blame the user: the generated header will have SWIFT_NOESCAPE on the parameter in the @protocol Fooable definition, but nobody looks at that and there’s no help whatsoever from the tools. Moreover, up until fairly recently, the Swift compiler didn’t treat @noescape closures any differently than @escaping closures, so the code would not break. The Swift compiler now (on master) optimizes away ref-count traffic for @noescape closures, and code that goes through this code path—i.e., Swift code that forms a closure and ends up calling -[MyClass fooWithCompletion:] via Fooable—will end up accessing memory that’s already been freed if the queued work happens later.

This problem doesn’t happen (silently) in pure Swift, because the compiler will complain if you try to save a @noescape closure somewhere for use later. The way to trigger this kind of problem in pure Swift is to abuse withoutActuallyEscaping:

func sneakyEscaping(completion: () -> Void) {
  withoutActuallyEscaping(completion) { completion in
    DispatchQueue.main.async(completion)
  }
}

However, this error will be (deterministically) caught at runtime, because withoutActuallyEscaping performs checking to ensure that the closure itself did not escape.

I see a couple of potential solutions here, which aren’t mutually exclusive:

  1. Implicitly use withoutActuallyEscaping when calling an @objc API from Swift: if a @noescape closure is being passed to an @objc method, perform the same checking logic performed by withoutActuallyEscaping. This won’t prevent the runtime failure, but it will make the runtime failure deterministic (rather than causing random memory corruption!) so the problems will be easier to spot.

  2. Warn about non-escaping closure parameters in @objc entry points that can be implemented in Objective-C: this would warn about any method in an @objc protocol that takes a closure that isn’t marked @escaping, as well as any non-final @objc method that takes a closure that isn’t marked @escaping. Examples:

  @objc protocol P {
     func foo(completion: () -> Void)   // warn: non-escaping function parameter in Objective-C entrypoint
     func bar(completion: @escaping () -> Void) // no warning
   }

   @objc class Foo : NSObject {
     @objc func method1(completion: () -> Void) { } // warn: non-escaping function parameter in Objective-C entrypoint
     @objc func method2(completion: @escaping () -> Void) { } // no warning
     @objc final func method3(completion: () -> Void) { } // no warning; can't override from Objective-C
       func method4(completion: () -> Void) { } // no warning; can't override from Objective-C
   }

We would probably want to bring back @noescape (it’s spelling was removed from the language by SE-0103) as a way to suppress the warning for those cases where you do want a non-escaping closure exposed to Objective-C. The warning would have two Fix-Its: one to add @escaping and one to silence the warning with @noescape.

Thoughts?

Doug


#2

It seems to me that solution 1 should be done regardless of whether solution 2 is, to match both people’s expectations around memory safety and the behaviour of withoutActuallyEscaping. Solution 2 is harder to evaluate, especially since it probably requires bringing back an already-removed keyword just to silence a warning that only occurs due to Objective-C interoperability. I think it comes down to how frequently this issue will arise in practice and how hard it will be to understand how to fix it.


(Howard Lovatt) #3

Both 1 and 2 sound good to me and also I can see why you would want @noescape.


(Slava Pestov) #4

I strongly prefer #1 because it does not increase the surface area of the language.


(Timothy J. Wood) #5

Why not make - (void)fooWithCompletion:(void (^)(void))completion not meet the protocol requirements, requiring NS_NOESCAPE be applied to the completion block type (and subsequently enforcing that to prevent passing it to async)?


(John McCall) #6

If we did bring back @noescape, it would have to be @nonEscaping, I think.

I agree with @jawbroken that solution 1 is a good safety check to be doing in general, and since it seems to solve the problem by making the failure much more reliable (though still not deterministic), all the better. We’d need some way of doing that check on block-representation functions, though.


(Arnold Schwaighofer) #7

The problem is the “escaped” @noescape swift closure. I think, to verify that the objective c closure has not escaped, we would store a non-trivial (vs a trivial) closure type in the block (and thereby have the block_descriptor perform copy/destroy_value operations like it does for escaping closures) and check it after the local block copy, that is passed to objective-c, is destroyed.

 %8 = convert_escape_to_noescape %0 : $@callee_guaranteed () -> () to $@noescape @callee_guaranteed () -> () // user: %11
 %8_verification = partial_apply %withoutActuallyEscapingThunk(%8) : @callee_guaranteed () -> ()
 strong_retain %8_verification // copy for block
 %9 = alloc_stack $@block_storage @noescape @callee_guaranteed () -> () // users: %15, %13, %10
 ///%10 = project_block_storage %9 : $*@block_storage @noescape @callee_guaranteed () -> () // user: %11
 %10 = project_block_storage %9 : $*@block_storage  @callee_guaranteed () -> () // user: %11
 ///store %8 to %10 : $*@noescape @callee_guaranteed () -> () // id: %11
 store %8_verification to %10 : $* @callee_guaranteed () -> () // id: %11
 // function_ref thunk for @callee_guaranteed () -> ()
 %12 = function_ref @reabstraction thunk helper from @callee_guaranteed () -> () to @callee_unowned @convention(block) () -> () : $@convention(c) (@inout_aliasable @block_storage @noescape @callee_guaranteed () -> ()) -> () // user: %13
 %13 = init_block_storage_header %9 : $*@block_storage @noescape @callee_guaranteed () -> (), invoke %12 : $@convention(c) (@inout_aliasable @block_storage @noescape @callee_guaranteed () -> ()) -> (), type $@convention(block) @noescape () -> () // user: %14
 %14 = copy_block %13 : $@convention(block) @noescape () -> () // users: %19, %18
 dealloc_stack %9 : $*@block_storage @noescape @callee_guaranteed () -> () // id: %15
 strong_release %0 : $@callee_guaranteed () -> () // id: %16
 %17 = objc_method %4 : $@opened("") DangerousEscaper, #DangerousEscaper.mightBeNaughty!1.foreign : <Self where Self : DangerousEscaper> (Self) -> (() -> ()) -> (), $@convention(objc_method) <τ_0_0 where τ_0_0 : DangerousEscaper> (@convention(block) @noescape () -> (), τ_0_0) -> () // user: %18
 %18 = apply %17<@opened("") DangerousEscaper>(%14, %4) : $@convention(objc_method) <τ_0_0 where τ_0_0 : DangerousEscaper> (@convention(block) @noescape () -> (), τ_0_0) -> () // type-defs: %4
 strong_release %14 : $@convention(block) @noescape () -> () // id: %19
 %f = is_escaping_closure %8_verification // we better have a ref-count of 1 otherwise the block was escaped.
 cond_fail %f
 ...

(Matthew Johnson) #8

#1 sounds good to me.

This isn’t directly relevant to Swift evolution, but did you consider what it would take to add SWIFT_NOESCAPE checking to Objective-C and require that when conforming to Swift protocols where it is specified? It may not be worth the effort but it would be the only way to actually prevent this problem while allowing Objective-C classes to conform to Swift protocols with non-escaping closure parameters.

Another option would be to not all non-escaping closure parameters in @objc protocols. The source breakage might be too significant to make a change like that but it would also prevent the problem.


(John McCall) #9

Unfortunately, I think we don’t have a positive escaping marker in ObjC, and warning on unannotated block parameters is problematic.


(Douglas Gregor) #10

@Timothy_J_Wood’s suggestion is probably the least-invasive warning we could produce. Essentially, if you’re implementing a method in Objective-C with an unannotated block parameter, and the corresponding declaration of that method is marked __attribute__((noescape)), produce a warning. In my example, that means we would warn here:

@implementation MyClass
- (void)fooWithCompletion:(void (^)(void))completion {  // warning: block is implicitly escaping, but declaration states that it is noescape
  dispatch_async(dispatch_get_main_queue(), completion);  // nothing will prevent you from actually writing this bad code, though
}
@end

and one would add the noescape attribute to the definition to silence the warning. This does have the nice advantage that you only get warnings in those places in Objective-C code where you wouldn’t have noticed the noescape, so at least it’s alerting developers to the problematic code.

Doug


(Matthew Johnson) #11

This sounds pretty reasonable to me.


(Fabian) #12

Any news on this? How is the state in released Swift 4.2?


(Douglas Gregor) #13

Swift 4.2 implements the runtime escaping check (solution #1 in my original post), and the corresponding Clang implements a warning for Objective-C @implementation methods with a parameter that doesn't mention __attribute__((noescape)) but clearly implement methods whose corresponding parameter is marked with __attribute__((noescape)).

Doug