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

Jump to bottom

[SR-6942] publishing a reference to self in deinit causes Swift to be memory unsafe #49490

Closed
weissi opened this issue Feb 7, 2018 · 8 comments · Fixed by #62191
Closed

[SR-6942] publishing a reference to self in deinit causes Swift to be memory unsafe #49490

weissi opened this issue Feb 7, 2018 · 8 comments · Fixed by #62191
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. memory safety Feature: memory safety runtime The Swift Runtime

Comments

@weissi
Copy link
Member

weissi commented Feb 7, 2018

Previous ID SR-6942
Radar rdar://problem/22790607
Original Reporter @weissi
Type Bug

Attachment: Download

Additional Detail from JIRA
Votes 1
Component/s Standard Library
Labels Bug, Runtime
Assignee None
Priority Medium

md5: 5af5d21f748a661d7e9705da05799bbb

Issue Description:

There are many different programs that show this behaviour but for example, let's use

class Foo {
}

class A {
    let foo: Foo
    init() {
        self.foo = Foo()
    }

    func assignSelfToGlobal() {
        globalA = self
        print("assigned self ref to global")
    }

    func printFoo() {
        print("printing self.foo")
        print(self.foo)
    }

    deinit {
        print("-> deinit")
        self.assignSelfToGlobal()
    }
}
var globalA: A?
({
_ = A()
    print("calling globalA?.printFoo")
    globalA?.printFoo()
    print("still around")
}())

this program assigns the dying self reference to a global in deinit (obviously a very bad idea). After the object has been de-initialised, we then call a method on the global which causes the program to crash in random ways... For example:

$ swift test.swift
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       EXC_I386_GPFLT
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libswiftCore.dylib              0x0000000102d84b26 swift::RefCounts<swift::RefCountBitsT<(swift::RefCountInlinedness)1> >::incrementSlow(swift::RefCountBitsT<(swift::RefCountInlinedness)1>, unsigned int) + 22
1   test                            0x0000000102a20dd8 A.printFoo() + 296
2   test                            0x0000000102a21095 closure #&#8203;1 in  + 309
3   test                            0x0000000102a209ef main + 31
4   libdyld.dylib                   0x00007fff58fc9015 start + 1

Thread 0 crashed with X86 Thread State (64-bit):
  rax: 0x0000000000000000  rbx: 0x0000000000000000  rcx: 0x0000000000000000  rdx: 0x0000000000000000
  rdi: 0x00007fbbbdf06418  rsi: 0x800007fbbbdf063d  rbp: 0x00007ffeed1e06b0  rsp: 0x00007ffeed1e06a0
   r8: 0x0000000000000001   r9: 0x00007fbbbdc02e40  r10: 0x0000000000000000  r11: 0x0000000000000005
  r12: 0x0000000000000000  r13: 0x00007fbbbdf063f0  r14: 0x0000000000000000  r15: 0x0000000000000000
  rip: 0x0000000102d84b26  rfl: 0x0000000000010246  cr2: 0x0000000102abd760
  
Logical CPU:     2
Error Code:      0x00000000
Trap Number:     13

the output of the program might be:

$ swiftc test.swift && ./test
-> deinit
assigned self ref to global
calling globalA?.printFoo
printing self.foo
Segmentation fault: 11

Shouldn't there be some assertion that at the very end of deinit the reference count is still 0?

Rewriting the program slightly to

class Foo {
}

class A {
    let foo: Int
    init() {
        self.foo = 12
    }

    func assignSelfToGlobal() {
        globalA = self
        //print("assigned self ref to global")
    }

    func printFoo() {
        //print("printing self.foo")
        print(self.foo)
    }

    deinit {
        //print("-> deinit")
        self.assignSelfToGlobal()
    }
}
var globalA: A?
({
_ = A()
    //print("calling globalA?.printFoo")
    globalA?.printFoo()
    //print("still around")
}())

I get actually pretty bad behaviour, running this in a loop it prints:

$ swiftc test.swift && for f in $(seq 20); do ./test; done
3
Segmentation fault: 11
3
12
3
Segmentation fault: 11
12
3
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11
2
Segmentation fault: 11
2
Segmentation fault: 11
3
Segmentation fault: 11
2
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11
3
Segmentation fault: 11

so sometimes it crashes, sometimes it prints 12, sometimes 3, sometimes 2...

$ swift -version
Apple Swift version 4.0.3 (swiftlang-900.0.71 clang-900.0.38)
Target: x86_64-apple-macosx10.9

but same for swift-4.1-DEVELOPMENT-SNAPSHOT-2018-01-10
@belkadan
Copy link
Contributor

belkadan commented Feb 7, 2018

I thought we had one. @gparker42, @mikeash?

@mikeash
Copy link
Contributor

mikeash commented Feb 7, 2018

Looks like we don't. swift_deallocObjectImpl should probably check and do a fatal error near the end.

@gparker42
Copy link
Mannequin

gparker42 mannequin commented Feb 8, 2018

IIRC we can't do that in the runtime because the compiler is omitting releases inside deinit for performance reasons.

@mikeash
Copy link
Contributor

mikeash commented Feb 8, 2018

Could we get the compiler to emit an assert that the count at the end of deinit is whatever it thinks it’s supposed to be?

@belkadan
Copy link
Contributor

belkadan commented Feb 8, 2018

shajrawi (JIRA User), do you know if this optimization is actually important? Catching escapes seems like it would be really useful.

@weissi
Copy link
Member Author

weissi commented Feb 8, 2018

I'm all for performance but this one defeats memory safety which is AFAIK something Swift claims to have (unless you use stuff with Unsafe in the name). If someone needs performance better than what Swift can offer safely, there's always -Ounchecked (which should in my mind be named -fdisable-safety-checks or something.

@weissi
Copy link
Member Author

weissi commented Mar 27, 2018

@swift-ci create

@gottesmm
Copy link
Member

@gparker42 We aren't doing that anymore I don't think b/c you put in an assert into the runtime.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@AnthonyLatsis AnthonyLatsis added memory safety Feature: memory safety and removed standard library Area: Standard library umbrella labels Dec 7, 2022
@AnthonyLatsis AnthonyLatsis linked a pull request Dec 8, 2022 that will close this issue
[Runtime] Fatal error if self escapes from deinit. #62191
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. memory safety Feature: memory safety runtime The Swift Runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Runtime] Fatal error if self escapes from deinit. mikeash/swift
5 participants
@mikeash @gottesmm @weissi @belkadan @AnthonyLatsis