Skip to content
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

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

Open
jckarter opened this issue Oct 13, 2018 · 4 comments

Comments

@jckarter
Copy link
Member

@jckarter jckarter commented Oct 13, 2018

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
Copy link
Member Author

@jckarter jckarter commented Oct 15, 2018

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

@jckarter
Copy link
Member Author

@jckarter jckarter commented Oct 15, 2018

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
Copy link
Member

@gottesmm gottesmm commented Oct 15, 2018

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
Copy link
Member Author

@jckarter jckarter commented Oct 16, 2018

Hopefully this fixes it: #19902

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants