[SR-8990] Miscompile (possible assertion failure) modifying nonmutating property on protocol

[jckarter]

Previous ID	SR-8990
Radar	rdar://problem/45274900
Original Reporter	@jckarter
Type	Bug

Attachment: Download

Additional Detail from JIRA

Votes	1
Component/s	Compiler
Labels	Bug, 4.2Regression
Assignee	@jckarter
Priority	Medium

md5: cc1eadc6334f9f313216b81e73193004

Issue Description:

Users report their app is crashing due to a use-after-free when modifying a nonmutating property defined on a protocol extension. Test case project here:

iZettle/Flow#34 (comment)

They note that a snapshot compiler with asserts on hits an assertion failure:

iZettle/Flow#34 (comment)

The problem occurs when a property defined in a protocol or protocol extension with a nonmutating setter is mutated indirectly, as in:

import CoreGraphics

protocol SomeProtocol { }
class SomeClass: SomeProtocol { }

extension SomeProtocol {
    var someGetter: CGPoint {
        nonmutating set { _ = self }
        get { return .zero }
    }
}

SomeClass().someGetter.x = 42 // releases SomeClass() too early

The problem can be worked around by separating the `get` and `set` of the nonmutating property into their own statements:

let someObject = SomeClass()
var temp = someObject.someGetter // first get the property
temp.x = 42 // update the value
someObject.someGetter = temp // then set the property

[jckarter]

Possibly another report of a similar bug: https://forums.swift.org/t/semantics-of-on-class-type/16990/2

[jckarter]

Here's a version of the test program from the Github discussion:

import CoreGraphics

protocol SomeProtocol { }
class SomeClass: SomeProtocol { }

extension SomeProtocol {
    var someGetter: CGPoint {
        nonmutating set { _ = self }
        get { return .zero }
    }
}

func test(y: CGFloat) {
  SomeClass().someGetter.x = y
}

If I compile this with swiftc -emit-sil-ownership -emit-silgen foo.swift, I see the following SIL:

sil hidden @$S3foo4test1yy12CoreGraphics7CGFloatV_tF : $@convention(thin) (CGFloat) -> () {
  ...
  // the result of SomeClass()
  %4 = apply %3(%2) : $@convention(method) (@thick SomeClass.Type) -> @owned SomeClass // users: %20, %7 
  ...
  store %4 to [init] %6 : $*SomeClass             // id: %7
  ...
  store %4 to [init] %19 : $*SomeClass            // id: %20
  ...
}

indicating that the value gets moved into a temporary buffer and thence consumed twice, once while invoking the getter and again while invoking the setter. There's no intervening control flow between the two stores—everything is one basic block—so I'm surprised the ownership verifier doesn't catch this. cc @gottesmm

[gottesmm]

JoeG and I spoke about this. I explained to him the ownership verifier is compiled out when assertions are disabled. So the flag will do nothing.

To confirm what the reporter ran into, I ran the test program against a 4-2 convergence snapshot and we hit a SILGen is forwarding a cleanup twice (i.e. two consumes of the same value) assert. See attached 4.2-convergence.log.

[jckarter]

Hopefully this fixes it: #19902