Use After Free on Node Reference issues

[Airyzz]

In SwiftGodot, it is easy to end up in a situation where you can attempt to use a node reference after it has been freed.

nodeReference = getChild("some_node")
nodeReference.queueFree()
// later
nodeReference.doThing() // Use after free! (╯°□°）╯︵ ┻━┻

It is also possible to create this situation in gdscript, however the key difference is that in GDScript, doing so will not crash the entire game, but will just log an error and continue. I believe it would be in everyones best interest if we could have this behaviour in SwiftGodot.

It is possible to detect this situation using GD.isInstanceValid() on a Variant created from the node reference:

nodeReference = getChild("some_node")
nodeReferenceVariant = Variant(nodeReference)
// later
if GD.isInstanceValid(nodeReferenceVariant) {
    // This should be safe!
    nodeReference.doThing()
}

note, that it is important to to create this variant at a time where you are 100% sure that the node reference is valid. if you create the variant at time of testing, you can end up with some unexpected behaviour:

nodeReference = getChild("some_node")
// later
// Here we are creating a Variant based off a reference which may have been freed.
// Sometimes this may work, but most of the time this is probably an incorrect result.
// In some cases, isInstanceValid will return true, even after nodeReference has been freed
// when this happens, if you take a look at the content of nodeReference, you may find that 
// it's suddenly pointing at a different node which was allocated after the expected value of nodeReference was freed,
// or it may point to something like [Wrapped:0]
if GD.isInstanceValid(Variant(nodeReference)) {
    nodeReference.doThing()
}

[Airyzz]

I have been using something like this to handle node references:

import SwiftGodot

class NodeReference<T: Node> {
    private var _ref: Node?
    private var _variant: Variant?

    var inner: Node? {
        get {
            if _ref == nil {
                return nil
            }

            if GD.isInstanceValid(instance: _variant!) {
                return _ref
            }

            printerr("Referenced node was no longer valid, clearing")

            _ref = nil
            return nil
        }

        set {
            printd("Setting node reference to: \(newValue)")
            _ref = newValue
        }
    }

    init(node: Node) {
        inner = node
        _variant = Variant(node)
    }
}

But i think this is not ideal, and would be good if something could be done inside SwiftGodot so this is taken care of for the user if at all possible

[migueldeicaza]

Thanks for sharing the details of this issue, let me explore a couple of options.

The range is:

• In Debug mode, we could create those behind the scenes and check on every use.
• We could see if we get called back in response to the free event, and we could flag the instance as destroyed, and then add checks for this.

[migueldeicaza]

I have added an isValid property to Object which should help mitigate the need for manually using a Variant to track it.

[Airyzz]

Thanks, i'll give that a test!

I also just want to show this, mainly to have a record of it somewhere, but i was able to confirm that node references being kept around after the node has been freed can suddenly change to different nodes entirely, if a new node is allocated in memory at the same place as the old node.

Here is console output of the contents of an array of Decal nodes I was using to recycle old bullet hole decals, as well as their addresses in memory:

[0] -> Optional(BulletHoleDecal:<Decal#2529567968002>) (Optional(0x000000000c1edc10))
[1] -> Optional(@Decal@189:<Decal#2529601522665>) (Optional(0x000000000c224240))
[2] -> Optional(@Decal@191:<Decal#2529635077093>) (Optional(0x000000001394dff0))
[3] -> Optional(@Decal@193:<Decal#2529668631335>) (Optional(0x0000000013955d70))

Then after changing scene, some of these decals are freed, and new nodes are allocated. These new nodes happened to be allocated in the same place as the decals, resulting in unexpected behaviour of random nodes being contained in the array:

[0] -> Optional(BulletHoleDecal:<Decal#6930315611170>) (Optional(0x000000000c1edc10))
[1] -> Optional(Line3D3:<Line3D#6905015571752>) (Optional(0x000000000c224240))
[2] -> Optional(Line3D4:<Line3D#6905065903403>) (Optional(0x000000001394dff0))
[3] -> Optional(Line3D5:<Line3D#6905116235054>) (Optional(0x0000000013955d70))

[Airyzz]

The new isValid property doesn't seem to work unfortunately, looking at the result of it inside my NodeReference hack, i am seeing the following

printd("Ref was valid: \(_ref?.isValid)")
printd("Variant was valid: \(GD.isInstanceValid(instance: _variant!))")

result:

Ref was valid: Optional(true)
Variant was valid: false

Seems there are times when the variant thinks its not valid but the Wrapped thinks its still valid

[Airyzz]

A potential solution which could be quite elegant, but im not sure will be possible, is if we can extend and override the behaviour of optional:

import SwiftGodot

public extension Optional where Wrapped: Node {
    static func == (lhs: Node?, rhs: _OptionalNilComparisonType) -> Bool {
        if lhs?.isValid == false {
            // this will make 'node == nil' return true when the node reference is invalid
            return true
        }

        // im not 100% sure how to handle default case so this is definitely incorrect!
        return false
    }
}

while this will work for node == nil checks im not sure if we can do a similar thing to make this work for other types of null check operations, like if let node, node?.doThing etc.

While i love the node == nil i think if we cant override the behaviour of the other operations, we might get in to confusing sitations like this:

// This would work fine!
if node != nil {
    node!.doThing()
}

// But this would still try to call doThing even if node.isValid is false! Bad!
node?.doThing()

[Airyzz]

Looks like operators ?. and !. are unforunately reserved and cannot be overloaded :(