Skip to content

Commit 444c3ee

Browse files
committed
[silgen] Eliminate lifetime gap caused by AutoreleasingUnsafeMutablePointer's LValue component set.
The problem here is that the AutoreleasingUnsafeMutablePointer's LValue component would given the following Swift: ``` public class C {} @inline(never) func updateC(_ p: AutoreleasingUnsafeMutablePointer<C>) -> () { p.pointee = C() } public func test() -> C { var cVar = C() updateC(&cVar) return cVar } ``` generate the following SIL as part of setting cVar after returning from updateC. ``` %18 = function_ref @$s11autorelease7updateCyySAyAA1CCGF : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () // user: %19 %19 = apply %18(%17) : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () %20 = load [trivial] %8 : $*@sil_unmanaged C // user: %21 %21 = unmanaged_to_ref %20 : $@sil_unmanaged C to $C // user: %22 %22 = copy_value %21 : $C // user: %23 assign %22 to %7 : $*C // id: %23 end_access %7 : $*C // id: %24 ``` Once we are in Canonical SIL, we get the following SIL: ``` %18 = function_ref @$s11autorelease7updateCyySAyAA1CCGF : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () // user: %19 %19 = apply %18(%17) : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () %20 = load [trivial] %8 : $*@sil_unmanaged C // user: %21 %21 = unmanaged_to_ref %20 : $@sil_unmanaged C to $C // user: %22 %22 = copy_value %21 : $C // user: %23 store %22 to [assign] %7 : $*C // id: %23 end_access %7 : $*C // id: %24 ``` which destroy hoisting then modifies by hoisting the destroy by the store assign before the copy_value: ``` %18 = function_ref @$s11autorelease7updateCyySAyAA1CCGF : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () // user: %19 %19 = apply %18(%17) : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () destroy_addr %7 : $*C %20 = load [trivial] %8 : $*@sil_unmanaged C // user: %21 %21 = unmanaged_to_ref %20 : $@sil_unmanaged C to $C // user: %22 %22 = copy_value %21 : $C // user: %23 store %22 to [init] %7 : $*C // id: %23 end_access %7 : $*C // id: %24 ``` Given the appropriate Swift code, one could have that %21 is actually already stored in %7 and has a ref count of 1. In such a case, the destroy_addr would cause %21 to be deallocated before we can retain the value. In order to fix this edge case as a bug fix, in the setter for AutoreleasingUnsafeMutablePointer, we introduce a mark_dependence from the copied value onto the memory. This ensures that destroy hoisting does not hoist the destroy_addr past the mark_dependence, yielding correctness. That is we generate the following SIL: ``` %18 = function_ref @$s11autorelease7updateCyySAyAA1CCGF : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () // user: %19 %19 = apply %18(%17) : $@convention(thin) (AutoreleasingUnsafeMutablePointer<C>) -> () %20 = load [trivial] %8 : $*@sil_unmanaged C // user: %21 %21 = unmanaged_to_ref %20 : $@sil_unmanaged C to $C // user: %22 %22 = copy_value %21 : $C // user: %23 %23 = mark_dependence %22 : $C on %7 : $*C assign %23 to %7 : $*C // id: %23 end_access %7 : $*C // id: %24 ``` For those unfamiliar, mark_dependence specifies that any destroy of the memory in %7 can not move before any use of %23. rdar://99402398
1 parent 0f48feb commit 444c3ee

File tree

3 files changed

+23
-4
lines changed

3 files changed

+23
-4
lines changed

lib/SILGen/SILGenExpr.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5556,8 +5556,14 @@ class AutoreleasingWritebackComponent : public LogicalPathComponent {
55565556
unowned->getType().castTo<UnmanagedStorageType>().getReferentType());
55575557
auto owned = SGF.B.createUnmanagedToRef(loc, unowned, strongType);
55585558
auto ownedMV = SGF.emitManagedRetain(loc, owned);
5559-
5560-
// Reassign the +1 storage with it.
5559+
5560+
// Then create a mark dependence in between the base and the ownedMV. This
5561+
// is important to ensure that the destroy of the assign is not hoisted
5562+
// above the retain. We are doing unmanaged things here so we need to be
5563+
// extra careful.
5564+
ownedMV = SGF.B.createMarkDependence(loc, ownedMV, base);
5565+
5566+
// Then reassign the mark dependence into the +1 storage.
55615567
ownedMV.assignInto(SGF, loc, base.getUnmanagedValue());
55625568
}
55635569

test/SILGen/foreign_errors.swift

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,17 @@ func test0() throws {
2828
// CHECK: [[RESULT:%.*]] = apply [[METHOD]]({{.*}}, [[SELF]])
2929

3030
// Writeback to the first temporary.
31+
//
32+
// NOTE: We need to a mark dependence here to ensure that the destroy
33+
// associated with the assign to ERR_TEMP0 is not hoisted above the copy_value
34+
// of T1_COPY. If we were to allow that, we could introduce a lifetime gap
35+
// causing potentially use after frees.
36+
//
3137
// CHECK: [[T0:%.*]] = load [trivial] [[ERR_TEMP1]]
3238
// CHECK: [[T1:%.*]] = unmanaged_to_ref [[T0]]
3339
// CHECK: [[T1_COPY:%.*]] = copy_value [[T1]]
34-
// CHECK: assign [[T1_COPY]] to [[ERR_TEMP0]]
40+
// CHECK: [[T1_COPY_DEP:%.*]] = mark_dependence [[T1_COPY]] : $Optional<NSError> on [[ERR_TEMP0]]
41+
// CHECK: assign [[T1_COPY_DEP]] to [[ERR_TEMP0]]
3542

3643
// Pull out the boolean value and compare it to zero.
3744
// CHECK: [[BOOL_OR_INT:%.*]] = struct_extract [[RESULT]]

test/SILGen/pointer_conversion.swift

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,13 @@ func classInoutToPointer() {
250250
// CHECK: [[UNOWNED_OUT:%.*]] = load [trivial] [[WRITEBACK]]
251251
// CHECK: [[OWNED_OUT:%.*]] = unmanaged_to_ref [[UNOWNED_OUT]]
252252
// CHECK: [[OWNED_OUT_COPY:%.*]] = copy_value [[OWNED_OUT]]
253-
// CHECK: assign [[OWNED_OUT_COPY]] to [[WRITE2]]
253+
//
254+
// DISCUSSION: We need a mark_dependence here to ensure that the destroy of
255+
// the value in WRITE2 is not hoisted above the copy of OWNED_OUT. Otherwise,
256+
// we may have a use-after-free if ref counts are low enough.
257+
//
258+
// CHECK: [[OWNED_OUT_COPY_DEP:%.*]] = mark_dependence [[OWNED_OUT_COPY]] : $C on [[WRITE2]]
259+
// CHECK: assign [[OWNED_OUT_COPY_DEP]] to [[WRITE2]]
254260

255261
var cq: C? = C()
256262
takesPlusZeroOptionalPointer(&cq)

0 commit comments

Comments
 (0)