robusta icon indicating copy to clipboard operation
robusta copied to clipboard

TryFromJavaValue is not safe

Open uvlad7 opened this issue 7 months ago • 0 comments

Default implementation for TryFromJavaValue looks like the following:

                    #[automatically_derived]
                    impl<
                        'env: 'borrow,
                        'borrow,
                    > ::robusta_jni::convert::TryFromJavaValue<'env, 'borrow>
                    for RubyModule<'env, 'borrow> {
                        type Source = ::robusta_jni::jni::objects::JObject<'env>;
                        fn try_from(
                            source: Self::Source,
                            env: &'borrow ::robusta_jni::jni::JNIEnv<'env>,
                        ) -> ::robusta_jni::jni::errors::Result<Self> {
                            Ok(Self {
                                raw: ::robusta_jni::jni::objects::AutoLocal::new(
                                    env,
                                    source,
                                ),
                            })
                        }
                    }

it never fails and isn't actually safe. It allows to screw up with types and to write something like

    let xml_module: jni::RubyModule = robusta_jni::convert::TryFromJavaValue::try_from(JObject::from(env.new_string("Xml").unwrap()), &env).unwrap();

and it will compile and fail in runtime somewhere xml_module is used. If xml_module is used as a caller, java.lang.NoSuchMethodError will be - most likely - thrown, but if it's used as an argument, things become even worse, because Java doesn't check if arguments really match the signature provided, so it'll crash with some cryptic error - java.lang.NullPointerException: Cannot read the array length because "cache" is null - or even segfault.

So, maybe use is_instance_of/is_assignable_from check in TryFromJavaValue, at least in debug builds? Or mark try_from as unsafe?

(Yes, I see that things like JClass::from aren't safe and unsafe too, probably it should be changed too)

uvlad7 avatar Dec 28 '23 03:12 uvlad7