Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Fix dynamic Variant params stack constructions in JNI callbacks #76640

Merged

Conversation

shendo
Copy link
Contributor

@shendo shendo commented May 1, 2023

Emitting signals with params from Android plugins could crash due to object assignment with uninitialised mem.
Instead, use memnew_placement to construct into stack addresses.

This appears to be introduced in godot 4.0 from refactoring so not applicable for 3.x.

Java_org_godotengine_godot_plugin_GodotPlugin_nativeEmitSignal was main issue and whilst appeared to run fine with Armv8 ABI for me, consistently crashed when running Armv7.

Java_org_godotengine_godot_GodotLib_callobject was not affected due to constructing a nil Variant to the stack addresses before assignment.

Java_org_godotengine_godot_GodotLib_calldeferred looked to have same issue but not sure if actually used.

I've updated the above three calls for consistency.

Not sure the circumstances that GetObjectArrayElement could return null (out of java refs/mem?) but was being tested in some funcs. I've changed to an ERR_FAIL_NULL as I believe it better to abort the call than continue with partially converted params.

JNI Push/PopLocalFrame calls have been removed from call funcs as seem somewhat redundant here and could interfere with early return. May reduce local java refs capacity by a couple.

Fixes #75754.
Probably Fixes #69297.

Thanks to @Ajrarn777 for assistance in tracking this down.

@shendo shendo requested a review from a team as a code owner May 1, 2023 05:09
@Chaosus Chaosus added this to the 4.1 milestone May 1, 2023
…acks

Emitting signals with params from Android plugins could crash due to
object assignment with uninitialised mem.  Instead, use 'memnew_placement'
to construct into stack addresses.  Make similar JNI callbacks consistent.

Fixes godotengine#75754.
@shendo shendo force-pushed the android_plugin_crash_on_emit_signal branch from abaef95 to 92ade92 Compare May 1, 2023 06:32
@shendo
Copy link
Contributor Author

shendo commented May 1, 2023

Alternatively, could be rewritten safer using vectors, if willing to forgo object stack allocs. Eg. Something like?

        int count = env->GetArrayLength(j_signal_params);
 
-       Variant *variant_params = (Variant *)alloca(sizeof(Variant) * count);
+       std::vector<Variant> variant_params;
+       variant_params.reserve(count);
        const Variant **args = (const Variant **)alloca(sizeof(Variant *) * count);
 
        for (int i = 0; i < count; i++) {
                jobject j_param = env->GetObjectArrayElement(j_signal_params, i);
                ERR_FAIL_NULL(j_param);
-               memnew_placement(&variant_params[i], Variant(_jobject_to_variant(env, j_param)));
+               variant_params.push_back(_jobject_to_variant(env, j_param));
                args[i] = &variant_params[i];
                env->DeleteLocalRef(j_param);
        }

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@m4gr3d m4gr3d merged commit ee86505 into godotengine:master May 1, 2023
@shendo shendo deleted the android_plugin_crash_on_emit_signal branch May 1, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugins causing crashes when emitting signals with arguments in some devices having crash on android.
3 participants