Remove Desc<JClass> implementation for JObject and &JObject

[rib]

It was surprising that these called .get_object_class() and let code pass object instances in place of any class argument.

A simpler implementation that would instead transmute the reference from a JObject to a JClass (like GlobalRef does) would make it too easy to pass an instance as a class which can lead to undefined behaviour for some JNI apis (such as JNIEnv::new_object())

Fixes: #118

--

For reference; I looked at adding a unit test to verify that you can't just pass an instance as a class any more but realized that doesn't just lead to a recoverable jni error / exception - it's undefined behaviour (at least for NewObjectA) and trying this I see an error like: FATAL ERROR in native method: JNI received a class argument that is not a class and the process aborts.

Although it's not something changed by this PR; it seems worth highlighting that currently "safe" APIs like like new_object don't double check that a given JClass really corresponds to a class, even though they could trigger undefined behaviour if that's not the case. (further it's also notable that NewObjectA additionally requires that the class "must not refer to an array class" which isn't something that's checked)

That should generally be OK since it shouldn't really be possible to get a JClass that isn't a class without unsafe code but one exception to that currently is the Desc<JClass> implementation for GlobalRef that simply assumes its safe to transmute the global reference into a JClass (and so passing a global reference to an instance to JNIEnv::new_object can lead to undefined behaviour - with or without this change.)

We have discussed before that GlobalRef should ideally be revised at some point to not erase the type of reference it holds, and that could go some way to making things safer here.

A bit of a side thought, but considering the overlooked requirement for NewObjectA that the class "must not refer to an array class" makes me wonder if new_object should perhaps have its own wrapper type for a validated JClass that would additionally validate the class isn't an array but also allow the reference to be cached via a GlobalRef (that wouldn't erase the type) so this validation wouldn't need to be repeated if instantiating the same type. (This is just thinking out loud for something that should be dealt with as a separate issue though imho) edit: (redundant since NewObjectA won't be reached before failing to find an array class constructor)

Definition of Done

• There are no TODOs left in the code
• Change is covered by automated tests
• The coding guidelines are followed
• Public API has documentation
• User-visible changes are mentioned in the Changelog
• The continuous integration build passes

[argv-minus-one]

For reference; I looked at adding a unit test to verify that you can't just pass an instance as a class any more but realized that doesn't just lead to a recoverable jni error / exception - it's undefined behaviour (at least for NewObjectA) and trying this I see an error like: FATAL ERROR in native method: JNI received a class argument that is not a class and the process aborts.

That's probably because the tests use -Xcheck:jni, which adds checks for some JNI programming errors, apparently including this one, and reliably aborts the process instead of UB. Without -Xcheck:jni, though, that would indeed be UB.

Although it's not something changed by this PR; it seems worth highlighting that currently "safe" APIs like like new_object don't double check that a given JClass really corresponds to a class, even though they could trigger undefined behaviour if that's not the case. (further it's also notable that NewObjectA additionally requires that the class "must not refer to an array class" which isn't something that's checked)

Have you tried that? I would expect it to fail reliably without UB when GetMethodID tries to look up a constructor for the array class. Per the documentation for java.lang.Class.getConstructors, array classes don't have any constructors to look up.

That should generally be OK since it shouldn't really be possible to get a JClass that isn't a class without unsafe code

True, but note that it is possible to get a JClass for an array class without unsafe code.

[rib]

Have you tried that? I would expect it to fail reliably without UB when GetMethodID tries to look up a constructor for the array class. Per the documentation for java.lang.Class.getConstructors, array classes don't have any constructors to look up.

I didn't try it out yet - I just noticed that specific restriction when I ended up looking at the spec for NewObjectA.

It sounds quite likely that we'd get a reliable runtime error via the MethodID lookup, like you say, which would be convenient here so there wouldn't be any need to have other explicit checks that he class isn't an array class.

[rib]

I've added a test that confirms that env.new_object() returns with a runtime error if you try and pass it an array class.

[rib]

Just a quick ping @argv-minus-one - any objection to me landing this PR now?

[argv-minus-one]

Nope. Looks good to me.