Rid and APIs accepting it are now unsafe

[Bromeon]

Fixes #836.

Makes all GDNative API methods accepting at least one Rid parameter unsafe.

Overall changes in Rid:

• Remove operator_less()
• Rename is_valid() -> is_occupied()
• get_id() is now unsafe + null-checked (if the RID is dangling, Godot dereferences an invalid pointer)
• Fix PartialOrd impl
• Docs

[Bromeon]

This dug up some extra dirt, especially regarding operator_less()...

1. This is a rather weird function to be public. Do we really need it, as we have PartialOrd?

---

2. Both equality and less operators are unsafe because the Godot implementation looks like this:

godot_bool GDAPI godot_rid_operator_equal(const godot_rid *p_self, const godot_rid *p_b) {
	const RID *self = (const RID *)p_self;
	const RID *b = (const RID *)p_b;
	return *self == *b;
}

godot_bool GDAPI godot_rid_operator_less(const godot_rid *p_self, const godot_rid *p_b) {
	const RID *self = (const RID *)p_self;
	const RID *b = (const RID *)p_b;
	return *self < *b;
}

Thanks to the dereferencing, passing an empty Rid (null pointer) as well as a dangling pointer is insta-UB.
Now, we have PartialEq and PartialOrd, so the safe expressions rid == rid and rid < rid can both cause UB.
This is ungood.

---

3. Our current PartialOrd is broken.

impl PartialOrd for Rid {
    #[inline]
    fn partial_cmp(&self, other: &Rid) -> Option<Ordering> {
        unsafe {
            let native = (get_api().godot_rid_operator_less)(&self.0, &other.0);

            if native {
                Some(Ordering::Less)
            } else {
                Some(Ordering::Greater)
            }
        }
    }
}

When is Ordering::Equal (or at least None) used?

Let's say we have let a: Rid = ...; let b = a;
Now we have either a < b (or a > b) always being true, although two values are equal.
That again means that b < a (or b > a) is also always true (since the operand value can't matter, the position must).

This violates the duality and irreflexibility properties:

• a < b if and only if b > a
• !(a < a), !(a > a)

[Bromeon]

To sum up, both PartialEq and PartialOrd are currently unsafe, and the latter is entirely broken.

We have the following options:

1. Attempt a safe implementation for both traits. But all we have is

   #[repr(C)]
   #[derive(Debug, Default, Copy, Clone)]
   pub struct godot_rid {
       pub _dont_touch_that: [u8; 8usize],
   }

   So we would need to do an arbitrary byte comparison, of which we don't know if it actually fulfills a semantically meaningful equality/ordering relation.

   The 2nd problem with this is, that a user who might be used to certain == and < semantics from RID in GDScript, will be nicely surprised, if we implement them differently.

2. We fix PartialOrd but leave both traits unsafe. We let users run into the knive 🔪
   (this is mostly an issue for dangling pointers)

3. We remove both trait impls, plus ToVariantEq. This may restrict some use cases, like sorting, using as keys in (tree) maps, etc. But it's possible to work around with custom functions that define their own ordering using unsafe methods.

[chitoyuu]

This dug up some extra dirt, especially regarding operator_less()...

1. This is a rather weird function to be public. Do we really need it, as we have PartialOrd?

It's probably unnecessary, yes.

2. Both equality and less operators are unsafe because the Godot implementation looks like this:

godot_bool GDAPI godot_rid_operator_equal(const godot_rid *p_self, const godot_rid *p_b) {
	const RID *self = (const RID *)p_self;
	const RID *b = (const RID *)p_b;
	return *self == *b;
}

godot_bool GDAPI godot_rid_operator_less(const godot_rid *p_self, const godot_rid *p_b) {
	const RID *self = (const RID *)p_self;
	const RID *b = (const RID *)p_b;
	return *self < *b;
}

Thanks to the dereferencing, passing an empty Rid (null pointer) as well as a dangling pointer is insta-UB. Now, we have PartialEq and PartialOrd, so the safe expressions rid == rid and rid < rid can both cause UB. This is ungood.

I don't think that's the case here, unless further dereferencing happens in the operator implementations? We're calling godot_rid_operator_less on pointers to godot_rid/RIDs, not the structs themselves: (get_api().godot_rid_operator_less)(&self.0, &other.0).

As I understand it the C++ code seems to be simply comparing the pointers as numbers.

3. Our current PartialOrd is broken.

I suppose this will have be fixed by calling godot_rid_operator_equal in case _less is ambiguous.

[Bromeon]

As I understand it the C++ code seems to be simply comparing the pointers as numbers.

Indeed I missed that we're passing a raw pointer to godot_rid, somehow I never think of pointers when seeing & in Rust 🙂

But they're not numbers. I researched a bit more, godot_rid is defined like this:

typedef struct {
	uint8_t _dont_touch_that[GODOT_RID_SIZE];
} godot_rid;

(i.e. the C origin of the bindgen'ed code). That pointer const godot_rid* is then reinterpreted (transmuted) as the layout-compatible const RID*:

class RID {
	mutable RID_Data *_data;

public:
	_FORCE_INLINE_ RID_Data *get_data() const { return _data; }

	_FORCE_INLINE_ bool operator==(const RID &p_rid) const {
		return _data == p_rid._data;
	}
	_FORCE_INLINE_ bool operator<(const RID &p_rid) const {
		return _data < p_rid._data;
	}
	_FORCE_INLINE_ bool is_valid() const { return _data != nullptr; }

	_FORCE_INLINE_ uint32_t get_id() const { return _data ? _data->get_id() : 0; }

	_FORCE_INLINE_ RID() {
		_data = nullptr;
	}
};

So there is another nesting (RID_Data), but all those operators in fact only compare the pointers and not the values behind. So we compare two RID_Data* values (i.e. pointers):

	mutable RID_Data *_data;

	bool operator<(const RID &p_rid) const {
		return _data < p_rid._data;
	}

Now this is C++, where nothing is just the way you'd intuitively expect it to be, especially in the territory of UB. And better yet, to be sure you not only need to find the right laws among the 1853 pages, but you better have 200$+ ready to spend. I checked StackOverflow, where we can continue the treasure hunt:

Indirection through an invalid pointer value and passing an invalid pointer value to a deallocation function have undefined behavior. Any other use of an invalid pointer value has implementation-defined behavior.

Great, so in theory we'd need to check compiler handbooks. But I don't think we have to go that far, if dangling RIDs can be compared in Godot, it will work for us. In practice, comparing pointers should just work. So PartialEq and PartialOrd should be safe.

get_id() on the other hand still needs to be unsafe, because _data could potentially be dangling:

uint32_t get_id() const { return _data ? _data->get_id() : 0; }

[Bromeon]

Ok, all changes in Rid:

• Remove operator_less()
• Rename is_valid() -> is_occupied()
• get_id() is now unsafe + null-checked
• Fix PartialOrd impl
• Docs

[Bromeon]

bors r+

[bors]

Build succeeded:

• full-ci