GDExtension method calls should not pass nullptr args

[rune-scape]

fix for a nasty crash that happens when passing a null variant to any gdextension function parameter expecting any kind of Object *.
the GDExtensionMethodBind expects a pointer to an Object * but get_opaque_pointer returns a nullptr which it tries to dereference..

could also be fixed in godot-cpp with a nullptr check, but this seemed more in line with what the binding code expects

this or another fix should rlly be applied to 4.2, 4.1, nd 4.0
it can be easily cherrypicked for 4.2 but 4.1 and 4.0 have different binding code, ill make a PR for 4.1 when its apropriate

fixes #86478
fixes godotengine/godot-cpp#1056

[dsnopek]

Hm. I'll do some more "historical" investigation when I have a chance, but if we've really been encoding null objects this way in 4.0, 4.1, and 4.2, I'd be reluctant to change this in Godot, because if it's been consistent, this sounds more like a godot-cpp bug.

[rune-scape]

pretty sure a bug in godot itsself,, i mean it doesnt even reach the gdextension dll,,
it seems like a clash between what the VariantInternal stuff used to do and what the GDExtensionMethodBind expects when making a validated call,, in 4.1 the crash happens in the extension dll instead
edit: wait i read the crash wrong it does always happen in the godot cpp dll,, still think its a bug in how godot serves Object **s to GDExtensionMethodBind
i guess the ptr to nullptr could be a ptr to the variant itsself like it is here:
https://github.com/godotengine/godot/blob/8fb744ac230a3896bf354e3a82760a94dccde024/core/extension/gdextension.cpp#L239
but only if a null variant is guaranteed to be zeroed out ( ^ as long as ret_opaque points to valid memory it shouldnt really matter what it is bc its memory for a null return type, not getting read or written to, just maybe ^ )
and i cant look thru every file to make sure it is..
so this solution seemed pretty good

[rune-scape]

hmmm, the failed check is a cancelled ios build,,,

[dsnopek]

Sorry for taking so long to get back to this!

I didn't do any testing, but I skimmed the code from 4.0, 4.1 and 4.2, and it does appear that we've been encoding null Object * as nullptr in ptrcalls through all of those versions.

So, I'm personally against making this change in Godot. I think consistency with previous versions is important unless there is a clear advantage to changing the encoding, and I don't think there is in this case (in fact, I think the current encoding is slightly better because we don't have to dereference the Object ** to check for nullptr - it's just nullptr). While this change is unlikely to break anything, it's likely that all bindings other than godot-cpp are already doing a correct null check, and changing the encoding is going to make the extra check that they're doing redundant, but they probably won't remove it in order to retain compatibility with older Godot versions.

I think the crash should be fixed in godot-cpp instead

[rune-scape]

it does appear that we've been encoding null Object * as nullptr in ptrcalls through all of those versions.
So, I'm personally against making this change in Godot. I think consistency with previous versions is important unless there is a clear advantage to changing the encoding

i dont think this comment makes sense, this is a crash that happens when a validated call is made and a Nil value is encoded as an Object ** , a Nil value can only be passed to an Object * parameter, builtin types cannot be null ,, its been consistently broken in all of gdextension this has to be fixed in Every version ,, there is no consistency to follow , that doesnt mean it was intentional, and i dont think this is 'changing the encoding' i think its following it more closely

While this change is unlikely to break anything, it's likely that all bindings other than godot-cpp are already doing a correct null check, and changing the encoding is going to make the extra check that they're doing redundant, but they probably won't remove it in order to retain compatibility with older Godot versions.

it actually seems like D bindings Dont make this check,
https://github.com/godot-dlang/godot-dlang/blob/ff421674d9f775f9cc88e57a6ab625b172b97b4f/src/godot/api/wrap.d#L356
but godot-rust does,,, if im interpreting this correctly
https://github.com/godot-rust/gdext/blob/88d8ed0719eb70d2a5af370e9b0fa672acea032a/godot-core/src/obj/raw.rs#L464C12-L464C15

since its a validated call, i doubt a null check should be done ,, it would need to get added in a couple places, it seems unlikely that someone just forgot that, i was interpreting the GDExtensionObjectPtr * or Object ** as 'a non null pointer to a godot Object *' ,, like a reference, but is a C api, because all other builtin type pointers dont have a null check ,, none of the validated arg to pointer conversion stuff have a null check ,, but its not written out anywhere ,, just gotta ask whoever made this i guess ,,, vnen and/or reduz

[rune-scape]

@vnen i see your name and reduz in the commit log for gdextension validated ptrcall ,, do you have insight about this?
are GDExtensionObjectPtr *s meant to be nullable when making a validated call?
should this change be in godot-cpp instead?

[dsnopek]

Here's alternative PR godotengine/godot-cpp#1405 which fixes the issue in godot-cpp rather than Godot itself.

I've also added discussion of this issue to the agenda of the next GDExtension meeting. (If anyone is interested in participating, please stop by #gdextension on chat.godotengine.org for details about the GDExtension meetings.)

[dsnopek]

We discussed this at the GDExtension meeting, and felt that it would be great if @Bromeon (or other maintainers of GDExtension bindings) could weigh in on the current encoding of Object *. It would also be nice to hear from @vnen or @reduz on what the original intention of the encoding was, if you can recall. Mainly, it's a question of if the current encoding is "incorrect" (as it relates to the original intention for the encoding) and should be fixed, or if the encoding is OK (since it has at least been consistent) and godot-cpp should be updated to correctly handle it.

[lilizoey]

In godot rust, null objects in argument calls are passed as pointers to null object pointers in all version of godot except 4.0. In 4.0 we pass them directly as null pointers.

However we also dont use null objects directly all that frequently (im unsure if we ever do actually) when calling gdextension methods since we have an outstanding bug where direct method calls only allow for non-null objects. The current workaround there is to use Object.call instead which means we then pass null variants instead of null objects. So it may be that we're doing it wrong and just haven't noticed because we actually dont ever do it.

I dont think it would really impact much if we end up needing to switch to encoding them as null pointers instead.

[dsnopek]

(Adding this comment just to differentiate my personal opinion from what I wrote in my previous comment, which was the result of that GDExtension meeting, and isn't the same as my personal view.)

As I've stated previously, it's my opinion that we don't really gain anything by changing the encoding as proposed in this PR: both the current encoding and the proposed encoding are capable of representing a null Object * just fine.

The only advantage to changing the encoding is theoretical "correctness", if the proposed encoding more closely matches the original intention for the encoding (however, it's not totally clear what the original intention was).

My personal vote is still for keeping the current encoding. Regardless of the intention, it's the encoding we've been using in all versions of Godot 4.x, and it's capable of representing what we need it to, so I see no compelling reason to change it.

But we certainly could improve the overall situation by documenting the encoding, so that it's clearer to the implementors of GDExtension bindings.

[Bromeon]

As I've stated previously, it's my opinion that we don't really gain anything by changing the encoding as proposed in this PR: both the current encoding and the proposed encoding are capable of representing a null Object * just fine.

My personal vote is still for keeping the current encoding. Regardless of the intention, it's the encoding we've been using in all versions of Godot 4.x, and it's capable of representing what we need it to, so I see no compelling reason to change it.

That is reasonable, assuming that this is not a limitation but rather choice of representation, and every binding is capable to represent null pointers. It would also prevent bindings from possibly needing to deal with different behavior across multiple API versions. As such, I'm with you here 🙂

[reduz]

Seems good, great catch.

[reduz]

Sorry I though this was a fix, so adding to the discussion: Regarding the intention of the encoding, I think (if I am not mistaken) it was that it passed a pointer to a pointer for returning, and a single pointer for argument passing. The problem is that this caused a lot of issues so I think at some point it was changed?

I honestly would use what work best for you, since you are the consumers of the API at this point.

It seems david has a better solution.

[reduz]

I checked #1405 and this looks like a better solution I guess, since each platform can handle it as preferred?

[dsnopek]

Superseded by PR godotengine/godot-cpp#1405