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] Don't include JNI_OnLoad in libSystem.Security.Cryptography.Native.Android.a #103231

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Jun 10, 2024

I'm working on a feature in .NET For Android which will allow us to link
all the native bits into a single shared library at application build
time. In order to make it possible, no archive (.a) may contain any
JNI_OnLoad functions (called by JavaVM when initializing a Java
extension DSO), because our runtime already contains one and there Can
be Only One(tm).

libSystem.Security.Cryptography.Native.Android is currently the only
BCL support native library which contains JNI_OnLoad and thus it
prevents us from linking it into our runtime. This PR changes things
a bit my moving the initialization code to a separate
function (AndroidCryptoNative_InitLibraryOnLoad ) which remains in the .a archive and
can be called by .NET For Android runtime from its own JNI_OnLoad as
well as by the libSystem.Security.Cryptography.Native.Android.so from
its JNI_OnLoad, which this PR moves to a separate source file that is
compiled only into the shared version of the crypto support library.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 10, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@@ -682,8 +682,7 @@ int GetEnumAsInt(JNIEnv *env, jobject enumObj)
return value;
}

JNIEXPORT jint JNICALL
JNI_OnLoad(JavaVM *vm, void *reserved)
jint init_library_on_load (JavaVM *vm, void *reserved)
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this something else that includes the lib name so we don't end up with duplicate init_library_on_load if we add JNI_OnLoad to another library.

Something like AndroidCryptoNative_InitLibraryOnLoad would fit the existing naming convention better

Copy link
Member

@akoeplinger akoeplinger Jun 10, 2024

Choose a reason for hiding this comment

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

also mark it as PALEXPORT in the header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't have to be exported. It's called by either android crypto's JNI_OnLoad when building a dynamic library or it will be called by our Android for .NET's one if we link it in statically. In neither case it needs to be exported, it's an internal call.

@steveisok
Copy link
Member

/cc @vitek-karas

@@ -682,7 +682,7 @@ int GetEnumAsInt(JNIEnv *env, jobject enumObj)
return value;
}

jint init_library_on_load (JavaVM *vm, void *reserved)
jint AndroidCryptoNative_InitLibraryOnLoad (JavaVM *vm, void *reserved)
{
(void)reserved;
LOG_INFO("JNI_OnLoad in pal_jni.c");
Copy link
Member

Choose a reason for hiding this comment

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

nit: this outdated log message might be confusing

Suggested change
LOG_INFO("JNI_OnLoad in pal_jni.c");
LOG_INFO("AndroidCryptoNative_InitLibraryOnLoad in pal_jni.c");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually planned on removing the message in a separate PR. It serves no real purpose and logcat calls are very expensive.

Copy link
Member

Choose a reason for hiding this comment

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

it has sometimes been useful in the past but LOG_DEBUG should be enough

@akoeplinger
Copy link
Member

@steveisok do you remember if the LibraryBuilder uses the static libs for Android? If yes we'll also need to fix it there.

grendello and others added 7 commits July 2, 2024 11:39
…phy.Native.Android.a`

I'm working on a feature in .NET For Android which will allow us to link
all the native bits into a single shared library at application build
time.  In order to make it possible, no archive (`.a`) may contain any
`JNI_OnLoad` functions (called by `JavaVM` when initializing a Java
extension DSO), because our runtime already contains one and there Can
be Only One(tm).

`libSystem.Security.Cryptography.Native.Android` is currently the only
BCL support native library which contains `JNI_OnLoad` and thus it
prevents us from linking it into our runtime.  This PR changes things
a bit my moving the initialization code to a separate
function (`init_library_on_load`) which remains in the `.a` archive and
can be called by `.NET For Android` runtime from its own `JNI_OnLoad` as
well as by the `libSystem.Security.Cryptography.Native.Android.so` from
its `JNI_OnLoad`, which this PR moves to a separate source file that is
compiled only into the shared version of the crypto support library.
@simonrozsival
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akoeplinger
Copy link
Member

I checked LibraryBuilder and it seems to use only the shared libraries so this is good to go.

@akoeplinger akoeplinger merged commit 048c8ed into dotnet:main Jul 9, 2024
123 of 136 checks passed
@grendello grendello deleted the crypto-no-onload-in-archive branch July 9, 2024 11:45
matouskozak added a commit to matouskozak/runtime that referenced this pull request Jul 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants