Fix use-after-free on substituting function type involving conditional ~Escapable with Escapable type #81678
Conversation
meg-gupta
requested review from
xymus,
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci test |
|
@swift-ci test |
| @@ -939,7 +940,8 @@ case TypeKind::Id: | |||
| }); | |||
|
|
|||
| if (didRemoveLifetimeDependencies) { | |||
| extInfo = extInfo->withLifetimeDependencies(substDependenceInfos); | |||
| extInfo = extInfo->withLifetimeDependencies( | |||
| ctx.AllocateCopy(substDependenceInfos)); | |||
There was a problem hiding this comment.
Maybe it's best to sink the AllocateCopy down into withLifetimeDependencies()
There was a problem hiding this comment.
Is there some rule for when withLifetimeDependencies needs its own allocation? Could that at least be commented in the API?
We call this in several other places and I don't know which need an allocation and which don't. e.g.
SILFunctionType::getSubstLifetimeDependencies
There was a problem hiding this comment.
It needs its own allocation if the LifetimeDependencies that you pass in are themselves not in the global AST context arena. It's safest to move the AllocateCopy into withLifetimeDependencies() itself, so that callers don't have to worry about creating an ExtInfo with a dangling pointer in it.
There was a problem hiding this comment.
I did not originally want to do this because withLifetimeDependencies is called from places with an already arena allocated pointer as the input arg, and proactively allocating within it would not allow sharing.
There was a problem hiding this comment.
perhaps the LifetimeDependencies objects should always be allocated and uniqued by the ASTContext then, so you'll pass them around by pointer and the pointer will always have global extent.
There was a problem hiding this comment.
I was hoping to use something akin to llvm::BumpPtrAllocatorImpl::identifyObject to ensure the passed input arg is safe in assert builds. Not everything is plumbed through to try that out.
I added a C++ delete overload for withLifetimeDependencies when input argument is SmallVectorImpl for now.
There was a problem hiding this comment.
perhaps the LifetimeDependencies objects should always be allocated and uniqued by the ASTContext then
@slavapestov This sounds promising. But maybe a big refactor for release/6.2
There was a problem hiding this comment.
I created rdar://151811669 (LifetimeDependencies objects should always be allocated and uniqued by the ASTContext)
…l ~Escapable with Escapable type
|
@swift-ci test |
While type substituting a conditionally ~Escapable type with an Escapable type, we have to drop lifetime dependencies from the function type. The newly constructed lifetime dependencies after dropping substituted dependencies was from a temporary whose lifetime is shorter than needed. Allocate it on the ASTContext like all other instances.
Fixes rdar://151769354