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

[weissi]

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]

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

[mikeash]

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

[gparker42]

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

[mikeash]

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]

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

[weissi]

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]

@swift-ci create

[gottesmm]

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