Skip to content

[Runtime] Fatal error if self escapes from deinit. #62191

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
merged 1 commit into from
Dec 7, 2022

Conversation

mikeash
Copy link
Contributor

@mikeash mikeash commented Nov 18, 2022

When deallocating a class instance, we check the retain count of the instance and error if it's greater than 1.

Self is allowed to be temporarily passed to other code from deinit, but there's no way to extend the lifetime of the object. Retaining it no longer extensd the lifetime. If self escapes from deinit, the result is a dangling pointer and eventual crash.

Instead of crashing randomly due to a dangling pointer, crash deliberately when destroying an object that has escaped.

rdar://93848484

@mikeash mikeash requested a review from al45tair November 18, 2022 20:58
@mikeash
Copy link
Contributor Author

mikeash commented Nov 18, 2022

This needs some more evaluation to see if there are any weird corner cases out there which currently manage to work, but would hit a fatal error with this check. It does pass the test suite locally, so that's a good start.

Copy link
Contributor

@al45tair al45tair left a comment

Choose a reason for hiding this comment

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

LGTM

@mikeash
Copy link
Contributor Author

mikeash commented Nov 28, 2022

@swift-ci please test

When deallocating a class instance, we check the retain count of the instance and error if it's greater than 1.

Self is allowed to be temporarily passed to other code from deinit, but there's no way to extend the lifetime of the object. Retaining it no longer extensd the lifetime. If self escapes from deinit, the result is a dangling pointer and eventual crash.

Instead of crashing randomly due to a dangling pointer, crash deliberately when destroying an object that has escaped.

rdar://93848484
@mikeash mikeash force-pushed the deinit-escape-assert branch from e2c766e to 108f780 Compare December 7, 2022 17:42
@mikeash
Copy link
Contributor Author

mikeash commented Dec 7, 2022

@swift-ci please test and merge

@mikeash
Copy link
Contributor Author

mikeash commented Dec 7, 2022

I've been running a bunch of stuff against this for the past week or so with no issues, so it seems like we should be in decent shape here.

@kavon kavon self-requested a review December 7, 2022 19:03
Copy link
Member

@kavon kavon left a comment

Choose a reason for hiding this comment

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

This is great! Also fixes #49490

@swift-ci swift-ci merged commit 571671f into swiftlang:main Dec 7, 2022
@AnthonyLatsis AnthonyLatsis added bug fix runtime The Swift Runtime memory safety Feature: memory safety labels Dec 7, 2022
@saagarjha
Copy link
Contributor

Hi! I am hitting this error in my code. I think it is sound (but it's using a bunch of Concurrency and a sprinkling of UnsafePointer, so maybe not?) so if you don't mind me asking, what is the invariant this is supposed to assert? I think this is actually being called on the release of a struct as well, so I'm kind of confused as what that might mean.

@al45tair
Copy link
Contributor

Does your struct contain any class references? Does it contain any COW struct types (e.g. Array) where there's a hidden class behind the scenes? This code only runs for refcounted objects (i.e. Swift class instances), and it's supposed to catch the case where a deinit has been called, and during that deinit something has incremented but not decremented the reference count on self (i.e. a reference to self has somehow escaped during deinit).

@al45tair
Copy link
Contributor

A simple example (that nobody would ever write) might be something like

var foo: SomeObject? = nil

...

class SomeObject {
  ...
  deinit {
    foo = self
  }
}

In practice it's much more likely that it would happen indirectly — that is, something you've done in deinit happens to gain access to self somehow and is stashing it somewhere.

@saagarjha
Copy link
Contributor

Maybe transitively via the AsyncStream? I think this is the object that it is upset about: https://github.com/saagarjha/unxip/blob/b59af49709e6921f6a1764d81601d585d77a6f06/unxip.swift#L16

@al45tair
Copy link
Contributor

I haven't had time to look at it in detail, but I don't see an obvious reason that that struct would be wrong itself, which makes me think maybe there's a bug in AsyncStream itself.

@saagarjha
Copy link
Contributor

I take that back, after some more testing it seems like the class type is actually Swift._DictionaryStorage<Swift.Int, unxip.Condition> which I assume backs this variable. That's a little more interesting, because I don't expect it to be deallocated early except at the end of the run.

@saagarjha
Copy link
Contributor

Yeah, it definitely seems like it's that. For my code, the pass that causes this to get hit moves the retain around. Here's what I see, using DEVELOPER_DIR=/path/to/Xcode_14.2.app swiftc -O -whole-module-optimization unxip.swift -parse-as-library -Xllvm -sil-opt-pass-count=38155 -Xllvm -sil-print-last | swift-demangle, which is the pass that causes my code to fail:

Before
  *** SIL function before  #38154, stage HighLevel,Function+EarlyLoopOpt, pass 41: RetainSinking (retain-sinking)
// ConcurrentStream.ensureWidth(index:)
sil hidden @$s5unxip16ConcurrentStreamC11ensureWidth5indexySi_tYaF : $@convention(method) @async <Element where Element : Sendable> (Int, @guaranteed ConcurrentStream<Element>) -> () {
// %0 "index"                                     // users: %8, %2
// %1 "self"                                      // users: %16, %79, %5, %4, %3
bb0(%0 : $Int, %1 : $ConcurrentStream<Element>):
  debug_value %0 : $Int, let, name "index", argno 1 // id: %2
  debug_value %1 : $ConcurrentStream<Element>, let, name "self", argno 2, implicit // id: %3
  hop_to_executor %1 : $ConcurrentStream<Element> // id: %4
  %5 = ref_element_addr %1 : $ConcurrentStream<Element>, #ConcurrentStream.batchSize // user: %6
  %6 = struct_element_addr %5 : $*Int, #Int._value // users: %84, %7
  %7 = load %6 : $*Builtin.Int64                  // users: %12, %9
  %8 = struct_extract %0 : $Int, #Int._value      // users: %85, %12, %9
  %9 = builtin "cmp_slt_Int64"(%8 : $Builtin.Int64, %7 : $Builtin.Int64) : $Builtin.Int1 // user: %10
  cond_br %9, bb7, bb1                            // id: %10

bb1:                                              // Preds: bb0
  %11 = integer_literal $Builtin.Int1, -1         // users: %67, %85, %40, %12
  %12 = builtin "ssub_with_overflow_Int64"(%8 : $Builtin.Int64, %7 : $Builtin.Int64, %11 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1) // users: %14, %13
  %13 = tuple_extract %12 : $(Builtin.Int64, Builtin.Int1), 0 // user: %25
  %14 = tuple_extract %12 : $(Builtin.Int64, Builtin.Int1), 1 // user: %15
  cond_fail %14 : $Builtin.Int1, "arithmetic overflow" // id: %15
  %16 = ref_element_addr %1 : $ConcurrentStream<Element>, #ConcurrentStream.widthConditions // users: %89, %17
  %17 = begin_access [read] [dynamic] %16 : $*Dictionary<Int, Condition> // users: %18, %23
  %18 = struct_element_addr %17 : $*Dictionary<Int, Condition>, #Dictionary._variant // user: %19
  %19 = struct_element_addr %18 : $*Dictionary<Int, Condition>._Variant, #Dictionary._Variant.object // user: %20
  %20 = struct_element_addr %19 : $*_BridgeStorage<__RawDictionaryStorage>, #_BridgeStorage.rawValue // user: %21
  %21 = load %20 : $*Builtin.BridgeObject         // users: %81, %26, %22
  strong_retain %21 : $Builtin.BridgeObject       // id: %22
  end_access %17 : $*Dictionary<Int, Condition>   // id: %23
  %24 = alloc_stack $Condition                    // users: %80, %52, %56, %82, %50
  %25 = struct $Int (%13 : $Builtin.Int64)        // user: %36
  %26 = unchecked_ref_cast %21 : $Builtin.BridgeObject to $__RawDictionaryStorage // users: %42, %36, %28, %27
  %27 = struct $_NativeDictionary<Int, Condition> (%26 : $__RawDictionaryStorage) // user: %51
  %28 = ref_element_addr %26 : $__RawDictionaryStorage, #__RawDictionaryStorage._count // user: %29
  %29 = struct_element_addr %28 : $*Int, #Int._value // user: %30
  %30 = load %29 : $*Builtin.Int64                // user: %31
  %31 = builtin "assumeNonNegative_Int64"(%30 : $Builtin.Int64) : $Builtin.Int64 // user: %33
  %32 = integer_literal $Builtin.Int64, 0         // user: %33
  %33 = builtin "cmp_eq_Int64"(%31 : $Builtin.Int64, %32 : $Builtin.Int64) : $Builtin.Int1 // user: %34
  cond_fail %33 : $Builtin.Int1, "Unexpectedly found nil while unwrapping an Optional value" // id: %34
  // function_ref specialized __RawDictionaryStorage.find<A>(_:)
  %35 = function_ref @$ss22__RawDictionaryStorageC4findys10_HashTableV6BucketV6bucket_Sb5foundtxSHRzlFSi_Tg5 : $@convention(method) (Int, @guaranteed __RawDictionaryStorage) -> (_HashTable.Bucket, Bool) // user: %36
  %36 = apply %35(%25, %26) : $@convention(method) (Int, @guaranteed __RawDictionaryStorage) -> (_HashTable.Bucket, Bool) // users: %38, %37
  %37 = tuple_extract %36 : $(_HashTable.Bucket, Bool), 0 // user: %45
  %38 = tuple_extract %36 : $(_HashTable.Bucket, Bool), 1 // user: %39
  %39 = struct_extract %38 : $Bool, #Bool._value  // user: %40
  %40 = builtin "xor_Int1"(%39 : $Builtin.Int1, %11 : $Builtin.Int1) : $Builtin.Int1 // user: %41
  cond_fail %40 : $Builtin.Int1, "Unexpectedly found nil while unwrapping an Optional value" // id: %41
  %42 = ref_element_addr %26 : $__RawDictionaryStorage, #__RawDictionaryStorage._rawValues // user: %43
  %43 = struct_element_addr %42 : $*UnsafeMutableRawPointer, #UnsafeMutableRawPointer._rawValue // user: %44
  %44 = load %43 : $*Builtin.RawPointer           // user: %48
  %45 = struct_extract %37 : $_HashTable.Bucket, #_HashTable.Bucket.offset // user: %46
  %46 = struct_extract %45 : $Int, #Int._value    // user: %47
  %47 = builtin "truncOrBitCast_Int64_Word"(%46 : $Builtin.Int64) : $Builtin.Word // user: %49
  %48 = pointer_to_address %44 : $Builtin.RawPointer to [strict] $*Condition // user: %49
  %49 = index_addr %48 : $*Condition, %47 : $Builtin.Word // user: %50
  copy_addr %49 to [initialization] %24 : $*Condition // id: %50
  fix_lifetime %27 : $_NativeDictionary<Int, Condition> // id: %51
  debug_value %24 : $*Condition, let, name "self", argno 1, implicit, expr op_deref // id: %52
  %53 = enum $Optional<Builtin.Executor>, #Optional.none!enumelt // users: %71, %54
  hop_to_executor %53 : $Optional<Builtin.Executor> // id: %54
  %55 = alloc_stack [lexical] $AsyncStream<()>.Iterator, var, name "$generator", implicit // users: %78, %77, %70, %62
  %56 = struct_element_addr %24 : $*Condition, #Condition.stream // user: %58
  %57 = alloc_stack $Optional<AsyncStream<()>>    // users: %59, %64, %60, %58
  copy_addr %56 to [initialization] %57 : $*Optional<AsyncStream<()>> // id: %58
  switch_enum_addr %57 : $*Optional<AsyncStream<()>>, case #Optional.some!enumelt: bb2, case #Optional.none!enumelt: bb3 // id: %59

bb2:                                              // Preds: bb1
  %60 = unchecked_take_enum_data_addr %57 : $*Optional<AsyncStream<()>>, #Optional.some!enumelt // users: %63, %62
  // function_ref AsyncStream.makeAsyncIterator()
  %61 = function_ref @$sScS17makeAsyncIteratorScS0C0Vyx_GyF : $@convention(method) <τ_0_0> (@in_guaranteed AsyncStream<τ_0_0>) -> @out AsyncStream<τ_0_0>.Iterator // user: %62
  %62 = apply %61<()>(%55, %60) : $@convention(method) <τ_0_0> (@in_guaranteed AsyncStream<τ_0_0>) -> @out AsyncStream<τ_0_0>.Iterator
  destroy_addr %60 : $*AsyncStream<()>            // id: %63
  dealloc_stack %57 : $*Optional<AsyncStream<()>> // id: %64
  // function_ref AsyncStream.Iterator.next()
  %65 = function_ref @$sScS8IteratorV4nextxSgyYaF : $@convention(method) @async <τ_0_0> (@inout AsyncStream<τ_0_0>.Iterator) -> @out Optional<τ_0_0> // user: %70
  br bb4                                          // id: %66

bb3:                                              // Preds: bb1
  cond_fail %11 : $Builtin.Int1, "Unexpectedly found nil while implicitly unwrapping an Optional value" // id: %67
  unreachable                                     // id: %68

bb4:                                              // Preds: bb5 bb2
  %69 = alloc_stack $Optional<()>                 // users: %73, %72, %70
  %70 = apply %65<()>(%69, %55) : $@convention(method) @async <τ_0_0> (@inout AsyncStream<τ_0_0>.Iterator) -> @out Optional<τ_0_0>
  hop_to_executor %53 : $Optional<Builtin.Executor> // id: %71
  %72 = load %69 : $*Optional<()>                 // user: %74
  dealloc_stack %69 : $*Optional<()>              // id: %73
  switch_enum %72 : $Optional<()>, case #Optional.some!enumelt: bb5, case #Optional.none!enumelt: bb6 // id: %74

bb5(%75 : $()):                                   // Preds: bb4
  br bb4                                          // id: %76

bb6:                                              // Preds: bb4
  destroy_addr %55 : $*AsyncStream<()>.Iterator   // id: %77
  dealloc_stack %55 : $*AsyncStream<()>.Iterator  // id: %78
  hop_to_executor %1 : $ConcurrentStream<Element> // id: %79
  destroy_addr %24 : $*Condition                  // id: %80
  strong_release %21 : $Builtin.BridgeObject      // id: %81
  dealloc_stack %24 : $*Condition                 // id: %82
  %83 = alloc_stack $Optional<Condition>          // users: %92, %94, %95
  %84 = load %6 : $*Builtin.Int64                 // user: %85
  %85 = builtin "ssub_with_overflow_Int64"(%8 : $Builtin.Int64, %84 : $Builtin.Int64, %11 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1) // users: %87, %86
  %86 = tuple_extract %85 : $(Builtin.Int64, Builtin.Int1), 0 // user: %91
  %87 = tuple_extract %85 : $(Builtin.Int64, Builtin.Int1), 1 // user: %88
  cond_fail %87 : $Builtin.Int1, "arithmetic overflow" // id: %88
  %89 = begin_access [modify] [dynamic] %16 : $*Dictionary<Int, Condition> // users: %92, %93
  // function_ref specialized Dictionary.removeValue(forKey:)
  %90 = function_ref @$sSD11removeValue6forKeyq_Sgx_tFSi_5unxip9ConditionVTg5 : $@convention(method) (Int, @inout Dictionary<Int, Condition>) -> @out Optional<Condition> // user: %92
  %91 = struct $Int (%86 : $Builtin.Int64)        // user: %92
  %92 = apply %90(%83, %91, %89) : $@convention(method) (Int, @inout Dictionary<Int, Condition>) -> @out Optional<Condition>
  end_access %89 : $*Dictionary<Int, Condition>   // id: %93
  destroy_addr %83 : $*Optional<Condition>        // id: %94
  dealloc_stack %83 : $*Optional<Condition>       // id: %95
  br bb8                                          // id: %96

bb7:                                              // Preds: bb0
  br bb8                                          // id: %97

bb8:                                              // Preds: bb6 bb7
  %98 = tuple ()                                  // user: %99
  return %98 : $()                                // id: %99
} // end sil function '$s5unxip16ConcurrentStreamC11ensureWidth5indexySi_tYaF'
After
  *** SIL function after  #38154, stage HighLevel,Function+EarlyLoopOpt, pass 41: RetainSinking (retain-sinking)
// ConcurrentStream.ensureWidth(index:)
sil hidden @$s5unxip16ConcurrentStreamC11ensureWidth5indexySi_tYaF : $@convention(method) @async <Element where Element : Sendable> (Int, @guaranteed ConcurrentStream<Element>) -> () {
// %0 "index"                                     // users: %8, %2
// %1 "self"                                      // users: %16, %79, %5, %4, %3
bb0(%0 : $Int, %1 : $ConcurrentStream<Element>):
  debug_value %0 : $Int, let, name "index", argno 1 // id: %2
  debug_value %1 : $ConcurrentStream<Element>, let, name "self", argno 2, implicit // id: %3
  hop_to_executor %1 : $ConcurrentStream<Element> // id: %4
  %5 = ref_element_addr %1 : $ConcurrentStream<Element>, #ConcurrentStream.batchSize // user: %6
  %6 = struct_element_addr %5 : $*Int, #Int._value // users: %84, %7
  %7 = load %6 : $*Builtin.Int64                  // users: %12, %9
  %8 = struct_extract %0 : $Int, #Int._value      // users: %85, %12, %9
  %9 = builtin "cmp_slt_Int64"(%8 : $Builtin.Int64, %7 : $Builtin.Int64) : $Builtin.Int1 // user: %10
  cond_br %9, bb7, bb1                            // id: %10

bb1:                                              // Preds: bb0
  %11 = integer_literal $Builtin.Int1, -1         // users: %67, %85, %39, %12
  %12 = builtin "ssub_with_overflow_Int64"(%8 : $Builtin.Int64, %7 : $Builtin.Int64, %11 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1) // users: %14, %13
  %13 = tuple_extract %12 : $(Builtin.Int64, Builtin.Int1), 0 // user: %24
  %14 = tuple_extract %12 : $(Builtin.Int64, Builtin.Int1), 1 // user: %15
  cond_fail %14 : $Builtin.Int1, "arithmetic overflow" // id: %15
  %16 = ref_element_addr %1 : $ConcurrentStream<Element>, #ConcurrentStream.widthConditions // users: %89, %17
  %17 = begin_access [read] [dynamic] %16 : $*Dictionary<Int, Condition> // users: %18, %22
  %18 = struct_element_addr %17 : $*Dictionary<Int, Condition>, #Dictionary._variant // user: %19
  %19 = struct_element_addr %18 : $*Dictionary<Int, Condition>._Variant, #Dictionary._Variant.object // user: %20
  %20 = struct_element_addr %19 : $*_BridgeStorage<__RawDictionaryStorage>, #_BridgeStorage.rawValue // user: %21
  %21 = load %20 : $*Builtin.BridgeObject         // users: %61, %81, %25
  end_access %17 : $*Dictionary<Int, Condition>   // id: %22
  %23 = alloc_stack $Condition                    // users: %80, %51, %55, %82, %49
  %24 = struct $Int (%13 : $Builtin.Int64)        // user: %35
  %25 = unchecked_ref_cast %21 : $Builtin.BridgeObject to $__RawDictionaryStorage // users: %41, %35, %27, %26
  %26 = struct $_NativeDictionary<Int, Condition> (%25 : $__RawDictionaryStorage) // user: %50
  %27 = ref_element_addr %25 : $__RawDictionaryStorage, #__RawDictionaryStorage._count // user: %28
  %28 = struct_element_addr %27 : $*Int, #Int._value // user: %29
  %29 = load %28 : $*Builtin.Int64                // user: %30
  %30 = builtin "assumeNonNegative_Int64"(%29 : $Builtin.Int64) : $Builtin.Int64 // user: %32
  %31 = integer_literal $Builtin.Int64, 0         // user: %32
  %32 = builtin "cmp_eq_Int64"(%30 : $Builtin.Int64, %31 : $Builtin.Int64) : $Builtin.Int1 // user: %33
  cond_fail %32 : $Builtin.Int1, "Unexpectedly found nil while unwrapping an Optional value" // id: %33
  // function_ref specialized __RawDictionaryStorage.find<A>(_:)
  %34 = function_ref @$ss22__RawDictionaryStorageC4findys10_HashTableV6BucketV6bucket_Sb5foundtxSHRzlFSi_Tg5 : $@convention(method) (Int, @guaranteed __RawDictionaryStorage) -> (_HashTable.Bucket, Bool) // user: %35
  %35 = apply %34(%24, %25) : $@convention(method) (Int, @guaranteed __RawDictionaryStorage) -> (_HashTable.Bucket, Bool) // users: %37, %36
  %36 = tuple_extract %35 : $(_HashTable.Bucket, Bool), 0 // user: %44
  %37 = tuple_extract %35 : $(_HashTable.Bucket, Bool), 1 // user: %38
  %38 = struct_extract %37 : $Bool, #Bool._value  // user: %39
  %39 = builtin "xor_Int1"(%38 : $Builtin.Int1, %11 : $Builtin.Int1) : $Builtin.Int1 // user: %40
  cond_fail %39 : $Builtin.Int1, "Unexpectedly found nil while unwrapping an Optional value" // id: %40
  %41 = ref_element_addr %25 : $__RawDictionaryStorage, #__RawDictionaryStorage._rawValues // user: %42
  %42 = struct_element_addr %41 : $*UnsafeMutableRawPointer, #UnsafeMutableRawPointer._rawValue // user: %43
  %43 = load %42 : $*Builtin.RawPointer           // user: %47
  %44 = struct_extract %36 : $_HashTable.Bucket, #_HashTable.Bucket.offset // user: %45
  %45 = struct_extract %44 : $Int, #Int._value    // user: %46
  %46 = builtin "truncOrBitCast_Int64_Word"(%45 : $Builtin.Int64) : $Builtin.Word // user: %48
  %47 = pointer_to_address %43 : $Builtin.RawPointer to [strict] $*Condition // user: %48
  %48 = index_addr %47 : $*Condition, %46 : $Builtin.Word // user: %49
  copy_addr %48 to [initialization] %23 : $*Condition // id: %49
  fix_lifetime %26 : $_NativeDictionary<Int, Condition> // id: %50
  debug_value %23 : $*Condition, let, name "self", argno 1, implicit, expr op_deref // id: %51
  %52 = enum $Optional<Builtin.Executor>, #Optional.none!enumelt // users: %71, %53
  hop_to_executor %52 : $Optional<Builtin.Executor> // id: %53
  %54 = alloc_stack [lexical] $AsyncStream<()>.Iterator, var, name "$generator", implicit // users: %78, %77, %70, %62
  %55 = struct_element_addr %23 : $*Condition, #Condition.stream // user: %57
  %56 = alloc_stack $Optional<AsyncStream<()>>    // users: %58, %64, %59, %57
  copy_addr %55 to [initialization] %56 : $*Optional<AsyncStream<()>> // id: %57
  switch_enum_addr %56 : $*Optional<AsyncStream<()>>, case #Optional.some!enumelt: bb2, case #Optional.none!enumelt: bb3 // id: %58

bb2:                                              // Preds: bb1
  %59 = unchecked_take_enum_data_addr %56 : $*Optional<AsyncStream<()>>, #Optional.some!enumelt // users: %63, %62
  // function_ref AsyncStream.makeAsyncIterator()
  %60 = function_ref @$sScS17makeAsyncIteratorScS0C0Vyx_GyF : $@convention(method) <τ_0_0> (@in_guaranteed AsyncStream<τ_0_0>) -> @out AsyncStream<τ_0_0>.Iterator // user: %62
  strong_retain %21 : $Builtin.BridgeObject       // id: %61
  %62 = apply %60<()>(%54, %59) : $@convention(method) <τ_0_0> (@in_guaranteed AsyncStream<τ_0_0>) -> @out AsyncStream<τ_0_0>.Iterator
  destroy_addr %59 : $*AsyncStream<()>            // id: %63
  dealloc_stack %56 : $*Optional<AsyncStream<()>> // id: %64
  // function_ref AsyncStream.Iterator.next()
  %65 = function_ref @$sScS8IteratorV4nextxSgyYaF : $@convention(method) @async <τ_0_0> (@inout AsyncStream<τ_0_0>.Iterator) -> @out Optional<τ_0_0> // user: %70
  br bb4                                          // id: %66

bb3:                                              // Preds: bb1
  cond_fail %11 : $Builtin.Int1, "Unexpectedly found nil while implicitly unwrapping an Optional value" // id: %67
  unreachable                                     // id: %68

bb4:                                              // Preds: bb5 bb2
  %69 = alloc_stack $Optional<()>                 // users: %73, %72, %70
  %70 = apply %65<()>(%69, %54) : $@convention(method) @async <τ_0_0> (@inout AsyncStream<τ_0_0>.Iterator) -> @out Optional<τ_0_0>
  hop_to_executor %52 : $Optional<Builtin.Executor> // id: %71
  %72 = load %69 : $*Optional<()>                 // user: %74
  dealloc_stack %69 : $*Optional<()>              // id: %73
  switch_enum %72 : $Optional<()>, case #Optional.some!enumelt: bb5, case #Optional.none!enumelt: bb6 // id: %74

bb5(%75 : $()):                                   // Preds: bb4
  br bb4                                          // id: %76

bb6:                                              // Preds: bb4
  destroy_addr %54 : $*AsyncStream<()>.Iterator   // id: %77
  dealloc_stack %54 : $*AsyncStream<()>.Iterator  // id: %78
  hop_to_executor %1 : $ConcurrentStream<Element> // id: %79
  destroy_addr %23 : $*Condition                  // id: %80
  strong_release %21 : $Builtin.BridgeObject      // id: %81
  dealloc_stack %23 : $*Condition                 // id: %82
  %83 = alloc_stack $Optional<Condition>          // users: %92, %94, %95
  %84 = load %6 : $*Builtin.Int64                 // user: %85
  %85 = builtin "ssub_with_overflow_Int64"(%8 : $Builtin.Int64, %84 : $Builtin.Int64, %11 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1) // users: %87, %86
  %86 = tuple_extract %85 : $(Builtin.Int64, Builtin.Int1), 0 // user: %91
  %87 = tuple_extract %85 : $(Builtin.Int64, Builtin.Int1), 1 // user: %88
  cond_fail %87 : $Builtin.Int1, "arithmetic overflow" // id: %88
  %89 = begin_access [modify] [dynamic] %16 : $*Dictionary<Int, Condition> // users: %92, %93
  // function_ref specialized Dictionary.removeValue(forKey:)
  %90 = function_ref @$sSD11removeValue6forKeyq_Sgx_tFSi_5unxip9ConditionVTg5 : $@convention(method) (Int, @inout Dictionary<Int, Condition>) -> @out Optional<Condition> // user: %92
  %91 = struct $Int (%86 : $Builtin.Int64)        // user: %92
  %92 = apply %90(%83, %91, %89) : $@convention(method) (Int, @inout Dictionary<Int, Condition>) -> @out Optional<Condition>
  end_access %89 : $*Dictionary<Int, Condition>   // id: %93
  destroy_addr %83 : $*Optional<Condition>        // id: %94
  dealloc_stack %83 : $*Optional<Condition>       // id: %95
  br bb8                                          // id: %96

bb7:                                              // Preds: bb0
  br bb8                                          // id: %97

bb8:                                              // Preds: bb6 bb7
  %98 = tuple ()                                  // user: %99
  return %98 : $()                                // id: %99
} // end sil function '$s5unxip16ConcurrentStreamC11ensureWidth5indexySi_tYaF'

I'm not entirely sure why this causes the failure (perhaps it is correct and some buggy code elsewhere is relying on the pre-optimization behavior) but it's definitely something here that's causing it to be upset.

@saagarjha
Copy link
Contributor

I think this bug is just diagnosing a more general issue in how captures are done across suspension points for actors: #61658 (comment). I'll go back to leaving you alone :)

@al45tair
Copy link
Contributor

al45tair commented Mar 20, 2023

I suspect the thing to do here is to file an issue detailing the problem, since it doesn't look like the issue is with your code (I've only glanced over it superficially, mind). I've pinged a couple of other folks to see if they'd take a look as it's the kind of thing that might interest them.

(Edit: Noticed that you'd already decided it was related to an existing issue; your comment appeared right after I pushed the button on mine :-))

@AnthonyLatsis AnthonyLatsis added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. and removed bug fix labels Apr 21, 2023
@mickeyl
Copy link

mickeyl commented May 31, 2023

Excellent. This improves the diagnostics a lot. I wonder though if the compiler could even statically warn if it encounters DispatchAsync or Task with self references being spawned during deinit?

@mikeash
Copy link
Contributor Author

mikeash commented May 31, 2023

Seems feasible. The overall trouble with static analysis is that it's semi-common (especially when using AppKit/UIKit) to pass self out to other methods in deinit that then do Mysterious Things to it, and you're essentially pinky promising that it doesn't really escape. That means the compiler can't be too strict about it. But capturing self with a Task or async dispatch is pretty much guaranteed to be wrong, so a warning should be doable for those specific cases.

KittyMac added a commit to KittyMac/rover that referenced this pull request May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. memory safety Feature: memory safety runtime The Swift Runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SR-6942] publishing a reference to self in deinit causes Swift to be memory unsafe
7 participants