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

Null pointer dereference (Android + JNI) #39

Closed
nadrolinux opened this issue May 11, 2022 · 5 comments
Closed

Null pointer dereference (Android + JNI) #39

nadrolinux opened this issue May 11, 2022 · 5 comments

Comments

@nadrolinux
Copy link

Hello,
At first, it's not a high priority issue in my opinion, but I found that Scapix may crash at:

// source/scapix/link/java/detail/api/ref.h
static jobject new_ref(jobject h) noexcept { return detail::env()->NewGlobalRef(h); }

Probably 'scope::local' and 'scope::weak' variants are also vulnerable to this issue.

When crash occurred I saw that 'jobject h' was nullptr, so 'NewLocalRef' caused abort with 'null pointer dereference' info. This issue may appear eg. when you store lambda with captured std::function to std::function (Scapix function stored here) and next you copy this std::function on another thread. I think that it may be relatated to detached JNI environment on another thread. I don't know if Scapix do some check like this one:
if (pVM->GetEnv((void**)&mEnvironment, JNI_VERSION_1_6) == JNI_EDETACHED && pVM->AttachCurrentThread(&mEnvironment, nullptr) == JNI_OK)
but if not it may be source of problem. However this issue may be relative easy solved by just store std::shared_ptr with std::function, so copy operate just on std::pointer instead of std::function, thats why I think that priority of this issue isn't high.

Best regards,
Patryk

@Boris-Rasin
Copy link
Member

As a workaround, you can call once in each thread created from C++:

scapix::link::java::attach_current_thread();

For convenience, I can also add generic version of this function (which will do nothing with other bridge languages):

scapix::init_thread();

I can also add optional automatic thread attachment:

target_compile_definitions(scapix PUBLIC SCAPIX_JNI_AUTO_ATTACH_THREAD)

I want to make it optional because it would add slight runtime overhead on each JNI call.

What do you think?

@nadrolinux
Copy link
Author

Hi,
I think that 3rd option with optional auto attachments is the best, because of automatic nature, but I agree that marked it as optional is better choice, because in this way we can still rely on options 1 and/or 2 if auto performance will be a problem. Please just remember that for option no. 2 you should also add 'scapix::exit_thread' or similar with internal 'scapix::link::java::detach_current_thread()' call.
Thanks for a quick response and good solutions as always.
Patryk

@Boris-Rasin
Copy link
Member

Commit: 0256e3f
Documentation: https://www.scapix.com/java_link/threads/

@Boris-Rasin
Copy link
Member

Released in cmodule v1.0.35:
https://github.com/scapix-com/cmodule/releases

@nadrolinux
Copy link
Author

Great, thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants