Skip to content

Commit 9ab8763

Browse files
authored
Merge pull request swiftlang#38965 from meg-gupta/fixdestroyhoisting
Fix use-after-free in DestroyHoisting
2 parents 9024f95 + 4c9cb0c commit 9ab8763

File tree

5 files changed

+167
-19
lines changed

5 files changed

+167
-19
lines changed

lib/SIL/Utils/MemoryLocations.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ bool MemoryLocations::analyzeLocationUsesRecursively(SILValue V, unsigned locIdx
357357
if (!handleNonTrivialProjections && hasInoutArgument(ApplySite(user)))
358358
return false;
359359
break;
360+
case SILInstructionKind::LoadBorrowInst:
361+
// Reborrows are not handled
362+
if (!cast<LoadBorrowInst>(user)->getUsersOfType<BranchInst>().empty())
363+
return false;
364+
break;
360365
case SILInstructionKind::InjectEnumAddrInst:
361366
case SILInstructionKind::SelectEnumAddrInst:
362367
case SILInstructionKind::ExistentialMetatypeInst:
@@ -367,7 +372,6 @@ bool MemoryLocations::analyzeLocationUsesRecursively(SILValue V, unsigned locIdx
367372
case SILInstructionKind::StoreInst:
368373
case SILInstructionKind::StoreBorrowInst:
369374
case SILInstructionKind::EndAccessInst:
370-
case SILInstructionKind::LoadBorrowInst:
371375
case SILInstructionKind::DestroyAddrInst:
372376
case SILInstructionKind::CheckedCastAddrBranchInst:
373377
case SILInstructionKind::UncheckedRefCastAddrInst:

lib/SILOptimizer/Transforms/DestroyHoisting.cpp

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -333,16 +333,21 @@ void DestroyHoisting::getUsedLocationsOfOperands(Bits &bits, SILInstruction *I)
333333
}
334334
}
335335

336-
// Set all bits of locations which instruction \p I is using. It's including
337-
// parent and sub-locations (see comment in getUsedLocationsOfAddr).
336+
// NOTE: All instructions handled in
337+
// MemoryLocations::analyzeLocationUsesRecursively should also be handled
338+
// explicitly here.
339+
// Set all bits of locations which instruction \p I is using.
340+
// It's including parent and sub-locations (see comment in
341+
// getUsedLocationsOfAddr).
338342
void DestroyHoisting::getUsedLocationsOfInst(Bits &bits, SILInstruction *I) {
339343
switch (I->getKind()) {
340-
case SILInstructionKind::EndBorrowInst:
341-
if (auto *LBI = dyn_cast<LoadBorrowInst>(
342-
cast<EndBorrowInst>(I)->getOperand())) {
344+
case SILInstructionKind::EndBorrowInst: {
345+
auto op = cast<EndBorrowInst>(I)->getOperand();
346+
if (auto *LBI = dyn_cast<LoadBorrowInst>(op)) {
343347
getUsedLocationsOfAddr(bits, LBI->getOperand());
344348
}
345349
break;
350+
}
346351
case SILInstructionKind::EndApplyInst:
347352
// Operands passed to begin_apply are alive throughout an end_apply ...
348353
getUsedLocationsOfOperands(bits, cast<EndApplyInst>(I)->getBeginApply());
@@ -351,6 +356,19 @@ void DestroyHoisting::getUsedLocationsOfInst(Bits &bits, SILInstruction *I) {
351356
// ... or abort_apply.
352357
getUsedLocationsOfOperands(bits, cast<AbortApplyInst>(I)->getBeginApply());
353358
break;
359+
case SILInstructionKind::EndAccessInst:
360+
getUsedLocationsOfOperands(bits,
361+
cast<EndAccessInst>(I)->getBeginAccess());
362+
break;
363+
// These instructions do not access the memory location for read/write
364+
case SILInstructionKind::StructElementAddrInst:
365+
case SILInstructionKind::TupleElementAddrInst:
366+
case SILInstructionKind::WitnessMethodInst:
367+
case SILInstructionKind::OpenExistentialAddrInst:
368+
break;
369+
case SILInstructionKind::InitExistentialAddrInst:
370+
case SILInstructionKind::InitEnumDataAddrInst:
371+
case SILInstructionKind::UncheckedTakeEnumDataAddrInst:
354372
case SILInstructionKind::SelectEnumAddrInst:
355373
case SILInstructionKind::ExistentialMetatypeInst:
356374
case SILInstructionKind::ValueMetatypeInst:
@@ -368,6 +386,7 @@ void DestroyHoisting::getUsedLocationsOfInst(Bits &bits, SILInstruction *I) {
368386
case SILInstructionKind::ApplyInst:
369387
case SILInstructionKind::TryApplyInst:
370388
case SILInstructionKind::YieldInst:
389+
case SILInstructionKind::SwitchEnumAddrInst:
371390
getUsedLocationsOfOperands(bits, I);
372391
break;
373392
case SILInstructionKind::DebugValueAddrInst:
@@ -765,18 +784,11 @@ class DestroyHoistingPass : public SILFunctionTransform {
765784
LLVM_DEBUG(llvm::dbgs() << "*** DestroyHoisting on function: "
766785
<< F->getName() << " ***\n");
767786

768-
bool EdgeChanged = splitAllCriticalEdges(*F, nullptr, nullptr);
769-
770787
DominanceAnalysis *DA = PM->getAnalysis<DominanceAnalysis>();
771788

772789
DestroyHoisting CM(F, DA);
773790
bool InstChanged = CM.hoistDestroys();
774791

775-
if (EdgeChanged) {
776-
// We split critical edges.
777-
invalidateAnalysis(SILAnalysis::InvalidationKind::FunctionBody);
778-
return;
779-
}
780792
if (InstChanged) {
781793
// We moved instructions.
782794
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);

test/SIL/memory_lifetime_failures.sil

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -538,3 +538,32 @@ bb0(%0 : $Bool):
538538
%10 = tuple ()
539539
return %10 : $()
540540
}
541+
542+
// MemoryLifetimeVerifier does not detect an error here due to reborrows
543+
sil [ossa] @test_load_borrow1 : $@convention(thin) (@in Optional<T>) -> () {
544+
bb0(%0 : $*Optional<T>):
545+
destroy_addr %0 : $*Optional<T>
546+
%1 = load_borrow %0 : $*Optional<T>
547+
br bb1(%1 : $Optional<T>)
548+
549+
bb1(%3 : @guaranteed $Optional<T>):
550+
end_borrow %3 : $Optional<T>
551+
br bb2
552+
553+
bb2:
554+
%r = tuple ()
555+
return %r : $()
556+
}
557+
558+
// CHECK: SIL memory lifetime failure in @test_load_borrow2: memory is not initialized, but should
559+
sil [ossa] @test_load_borrow2 : $@convention(thin) (@in Optional<T>) -> () {
560+
bb0(%0 : $*Optional<T>):
561+
destroy_addr %0 : $*Optional<T>
562+
%1 = load_borrow %0 : $*Optional<T>
563+
end_borrow %1 : $Optional<T>
564+
br bb1
565+
566+
bb1:
567+
%r = tuple ()
568+
return %r : $()
569+
}

test/SILOptimizer/destroy_hoisting.sil

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,3 +348,104 @@ bb0(%0 : $*X):
348348
return %res : $()
349349
}
350350

351+
// CHECK-LABEL: sil [ossa] @test_switch_enum_addr :
352+
// CHECK-NOT: destroy_addr
353+
// CHECK: switch_enum_addr
354+
// CHECK: } // end sil function 'test_switch_enum_addr'
355+
sil [ossa] @test_switch_enum_addr : $@convention(thin) (@in TwoCases) -> () {
356+
bb0(%0 : $*TwoCases):
357+
%2 = alloc_stack $TwoCases
358+
copy_addr %0 to [initialization] %2 : $*TwoCases
359+
switch_enum_addr %2 : $*TwoCases, case #TwoCases.A!enumelt: bb1, case #TwoCases.B!enumelt: bb2
360+
361+
bb1:
362+
destroy_addr %2 : $*TwoCases
363+
dealloc_stack %2 : $*TwoCases
364+
br bb3
365+
366+
bb2:
367+
destroy_addr %2 : $*TwoCases
368+
dealloc_stack %2 : $*TwoCases
369+
br bb3
370+
371+
bb3:
372+
destroy_addr %0 : $*TwoCases
373+
%r = tuple ()
374+
return %r : $()
375+
}
376+
377+
// CHECK-LABEL: sil [ossa] @test_load_borrow1 :
378+
// CHECK-NOT: destroy_addr
379+
// CHECK: load_borrow
380+
// CHECK-LABEL: } // end sil function 'test_load_borrow1'
381+
sil [ossa] @test_load_borrow1 : $@convention(thin) (@in TwoCases) -> () {
382+
bb0(%0 : $*TwoCases):
383+
%2 = alloc_stack $TwoCases
384+
copy_addr %0 to [initialization] %2 : $*TwoCases
385+
%ld = load_borrow %2 : $*TwoCases
386+
br bb1(%ld : $TwoCases)
387+
388+
bb1(%newld : @guaranteed $TwoCases):
389+
end_borrow %newld : $TwoCases
390+
br bb2
391+
392+
bb2:
393+
destroy_addr %2 : $*TwoCases
394+
dealloc_stack %2 : $*TwoCases
395+
br bb3
396+
397+
bb3:
398+
destroy_addr %0 : $*TwoCases
399+
%r = tuple ()
400+
return %r : $()
401+
}
402+
403+
// CHECK-LABEL: sil [ossa] @test_load_borrow2 :
404+
// CHECK-NOT: destroy_addr
405+
// CHECK: load_borrow
406+
// CHECK-LABEL: } // end sil function 'test_load_borrow2'
407+
sil [ossa] @test_load_borrow2 : $@convention(thin) (@in TwoCases) -> () {
408+
bb0(%0 : $*TwoCases):
409+
%2 = alloc_stack $TwoCases
410+
copy_addr %0 to [initialization] %2 : $*TwoCases
411+
%ld = load_borrow %2 : $*TwoCases
412+
cond_br undef, bb1, bb2
413+
414+
bb1:
415+
end_borrow %ld : $TwoCases
416+
br bb3
417+
418+
bb2:
419+
br bb2a(%ld : $TwoCases)
420+
421+
bb2a(%newld : @guaranteed $TwoCases):
422+
end_borrow %newld : $TwoCases
423+
br bb3
424+
425+
bb3:
426+
destroy_addr %2 : $*TwoCases
427+
dealloc_stack %2 : $*TwoCases
428+
br bb3
429+
430+
bb4:
431+
destroy_addr %0 : $*TwoCases
432+
%r = tuple ()
433+
return %r : $()
434+
}
435+
436+
// CHECK-LABEL: sil [ossa] @test_begin_access :
437+
// CHECK-NOT: destroy_addr
438+
// CHECK: begin_access
439+
// CHECK-LABEL: } // end sil function 'test_begin_access'
440+
sil [ossa] @test_begin_access : $@convention(thin) (@in X) -> () {
441+
bb0(%0 : $*X):
442+
%a = begin_access [read] [dynamic] %0 : $*X
443+
br bb1
444+
445+
bb1:
446+
end_access %a : $*X
447+
destroy_addr %0 : $*X
448+
%r = tuple ()
449+
return %r : $()
450+
}
451+

test/SILOptimizer/pointer_conversion.swift

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,12 @@ public func testMutableArray() {
5656
// CHECK: [[POINTER:%.+]] = struct $UnsafeMutableRawPointer (
5757
// CHECK-NEXT: [[DEP_POINTER:%.+]] = mark_dependence [[POINTER]] : $UnsafeMutableRawPointer on {{.*}} : $__ContiguousArrayStorageBase
5858
// CHECK: [[FN:%.+]] = function_ref @takesMutableRawPointer
59-
// CHECK-NEXT: apply [[FN]]([[DEP_POINTER]])
60-
// CHECK-NOT: release
59+
// CHECK: strong_retain {{%.+}} : ${{Builtin[.]BridgeObject|__ContiguousArrayStorageBase}}
60+
// CHECK: apply [[FN]]([[DEP_POINTER]])
6161
// CHECK-NOT: {{^bb[0-9]+:}}
6262
// CHECK: strong_release {{%.+}} : ${{Builtin[.]BridgeObject|__ContiguousArrayStorageBase}}
63-
// CHECK-NEXT: dealloc_stack {{%.+}} : $*Array<Int>
63+
// CHECK: strong_release {{%.+}} : ${{Builtin[.]BridgeObject|__ContiguousArrayStorageBase}}
64+
// CHECK: dealloc_stack {{%.+}} : $*Array<Int>
6465
// CHECK-NEXT: [[EMPTY:%.+]] = tuple ()
6566
// CHECK-NEXT: return [[EMPTY]]
6667
}
@@ -73,11 +74,12 @@ public func testMutableArrayToOptional() {
7374
// CHECK-NEXT: [[DEP_POINTER:%.+]] = mark_dependence [[POINTER]] : $UnsafeMutableRawPointer on {{.*}} : $__ContiguousArrayStorageBase
7475
// CHECK-NEXT: [[OPT_POINTER:%.+]] = enum $Optional<UnsafeMutableRawPointer>, #Optional.some!enumelt, [[DEP_POINTER]]
7576
// CHECK: [[FN:%.+]] = function_ref @takesOptMutableRawPointer
76-
// CHECK-NEXT: apply [[FN]]([[OPT_POINTER]])
77-
// CHECK-NOT: release
77+
// CHECK: strong_retain {{%.+}} : ${{Builtin[.]BridgeObject|__ContiguousArrayStorageBase}}
78+
// CHECK: apply [[FN]]([[OPT_POINTER]])
7879
// CHECK-NOT: {{^bb[0-9]+:}}
7980
// CHECK: strong_release {{%.+}} : ${{Builtin[.]BridgeObject|__ContiguousArrayStorageBase}}
80-
// CHECK-NEXT: dealloc_stack {{%.+}} : $*Array<Int>
81+
// CHECK: strong_release {{%.+}} : ${{Builtin[.]BridgeObject|__ContiguousArrayStorageBase}}
82+
// CHECK: dealloc_stack {{%.+}} : $*Array<Int>
8183
// CHECK-NEXT: [[EMPTY:%.+]] = tuple ()
8284
// CHECK-NEXT: return [[EMPTY]]
8385
}

0 commit comments

Comments
 (0)