API methods that take RID arguments need to be unsafe

[chitoyuu]

According to @Waridley on Discord:

Also, in investigating this, I realized it's super easy to invoke UB when working with RID's in release builds :ferris_sweat:

let vs = unsafe { VisualServer::godot_singleton() };
let canvas = vs.canvas_create();
let vp = vs.viewport_create();
vs.canvas_item_set_parent(vp, canvas); // crashes immediately
vs.canvas_item_set_parent(canvas, canvas); // crashes at shutdown

Fortunately, this still seems to require an unsafe block, and the engine catches it in debug builds, but the documentation for VisualServer only says that it's unsafe for multithreading reasons. Technically the unsafe part here is passing around typeless RID's which end up acting as raw pointers, even in a single-threaded context.

[Waridley]

An alternative could be to parameterize Rid based on which API it was created from, and provide unsafe methods to convert from, e.g. Rid<Unknown> to Rid<Canvas> when it's obtained from an unknown source. But that would require potentially significant manual work. (maybe? I mean, most methods seem to follow a consistent naming convention, so we might be able to write some logic to cover most cases...)

[chitoyuu]

An alternative could be to parameterize Rid based on which API it was created from...

Interesting idea. I took a glance at the methods and indeed they and their parameters are named quite consistently. Are Rids reference-counted, though? Because if we can get dangling pointers, then it doesn't matter as much if we know their types -- the methods would still have to be unsafe.

[Waridley]

Interesting idea. I took a glance at the methods and indeed they and their parameters are named quite consistently. Are Rids reference-counted, though? Because if we can get dangling pointers, then it doesn't matter as much if we know their types -- the methods would still have to be unsafe.

Ugh, I think you're right. There's an _owner pointer on RID_Data when debug info is not enabled, but not all methods check it. There are some checks against double-freeing, but I don't think it prevents use-after-free in all methods... 😒

[Bromeon]

Another thing to consider is that a small number of users may actually want type erasure. But this would be possible with a trait AbstractRid (basically Any + integer access; if implicit upcasts are intended), or the already suggested Rid<Unknown> (more explicit). For full type safety, we would need to type-ify the parameters as well.

For v0.10, should we simply mark the methods unsafe? It looks like type-ifying all Rids might be a larger endeavor, as we also need serialization (to/from variant), conversion between typed and untyped RIDs, etc. This might push the release further out, or maybe I'm overestimating it -- what do you think?

Regarding safety and double-free, this might also be relevant as a case in #808...

[chitoyuu]

For v0.10, should we simply mark the methods unsafe? It looks like type-ifying all Rids might be a larger endeavor, as we also need serialization (to/from variant), conversion between typed and untyped RIDs, etc.

I believe so. It isn't worth as much to type-ify RIds if it doesn't allow us to make the interface safe.