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

Regression in Log4j 2.22.0 on Android because of changes to getThreadContextClassLoader() in LoaderUtil #2129

Closed
centic9 opened this issue Dec 26, 2023 · 9 comments · Fixed by #2138
Assignees
Labels
bug Incorrect, unexpected, or unintended behavior of existing code runtime Specific to the runtime environment
Milestone

Comments

@centic9
Copy link
Member

centic9 commented Dec 26, 2023

Description

When upgrading an Android Application which has a transitive dependency on Log4j from log4j-core 2.21.1 to 2.22.0, it fails with an exception because it seems the method AccessController.doPrivileged() with the required parameters is not available on Android, but calls to it are introduced in LoaderUtil now via commit 80de816

This prevents us from upgrading log4j when including any Java libraries which depend on log4j-core in an Android application.

Is it possible to not introduce these changes in the 2.x series? The commit-message sounds like this was sort of "let's backport some stuff from 3.x branches".

Configuration

Version: 2.22.0

Operating system: Android application with min SDK Level 26

JDK: Android

Logs

java.lang.NoSuchMethodError: No static method doPrivileged(Ljava/security/PrivilegedAction;Ljava/security/AccessControlContext;[Ljava/security/Permission;)Ljava/lang/Object; in class Ljava/security/AccessController; or its super classes (declaration of 'java.security.AccessController' appears in /system/framework/core-oj.jar)
at org.apache.logging.log4j.util.LoaderUtil.getThreadContextClassLoader(LoaderUtil.java:155)
at org.apache.logging.log4j.util.LoaderUtil.findUrlResources(LoaderUtil.java:473)
at org.apache.logging.log4j.util.LoaderUtil.findResources(LoaderUtil.java:462)
at org.apache.logging.log4j.util.PropertyFilePropertySource.loadPropertiesFile(PropertyFilePropertySource.java:45)
at org.apache.logging.log4j.util.PropertyFilePropertySource.<init>(PropertyFilePropertySource.java:37)
at org.apache.logging.log4j.util.PropertiesUtil.<init>(PropertiesUtil.java:84)
at org.apache.logging.log4j.util.PropertiesUtil.<init>(PropertiesUtil.java:80)
at org.apache.logging.log4j.status.StatusLogger.<clinit>(StatusLogger.java:76)
at org.apache.logging.log4j.status.StatusLogger.getLogger(StatusLogger.java:149)
at org.apache.logging.log4j.LogManager.<clinit>(LogManager.java:60)
at org.apache.logging.log4j.LogManager.getLogger(LogManager.java:610)
at org.dstadler.poiandroidtest.poitest.ApplicationTest.<clinit>(ApplicationTest.java:17)

Reproduction

Can only be reproduced in Android applications using the Emulator.

There is a sample Android application "poi-on-android" which shows the issue:

centic9 added a commit to centic9/poi-on-android that referenced this issue Dec 26, 2023
@centic9
Copy link
Member Author

centic9 commented Dec 26, 2023

In order to provide a minimal reproducer I have prepared a branch at https://github.com/centic9/poi-on-android/tree/log4j_2_22_0_minimal_reproducer which is a minimal Android Application, upgraded to latest SDK level 34, which still shows the issue when either tests are executed or the resulting Android application is started.

@centic9
Copy link
Member Author

centic9 commented Dec 28, 2023 via email

@ppkarwasz
Copy link
Contributor

ppkarwasz commented Dec 28, 2023

Hi @centic9,

Thank You for Your issue report.

Is it possible to not introduce these changes in the 2.x series? The commit-message sounds like this was sort of "let's backport some stuff from 3.x branches".

That is the idea: we don't introduce breaking changes in 2.x. On the other hand we try to keep the two branches in sync, which helps backporting bug fixes from main to 2.x. The issue you pointed out is clearly a bug and we'll fix it.

Thank you for your Android test. If you agree, we'll add it to our test suite, so that we prevent this kind of problems in the future.

@centic9
Copy link
Member Author

centic9 commented Dec 29, 2023

Thanks for the explanation. Feel free to take from the reproducer whatever is useful.

@jvz
Copy link
Member

jvz commented Dec 29, 2023

We should probably just remove the SecurityManager support here, too, as we did in main.

@ppkarwasz
Copy link
Contributor

We should probably just remove the SecurityManager support here, too, as we did in main.

No, Matt, we can't. Wildfly still supports SecurityManager and expects Log4j 2.x to also support it.

Since we are struggling to support OSGi, JPMS, SecurityManager, Android and GraalVM and each of them has its own limitations, I would just propose to use Review-then-Commit on everything that concerns ServiceLoader, OSGi, and SecurityManager in 2.x, read: we don't change it unless it is broken. I have introduced and fixed enough bugs in these areas to know it is a minefield.

@centic9: Matt is working on removing SecurityManager entirely from 3.x. It should be ready in one of the next betas. Before we release it, could we ask you to check its compatibility with Android?

@jvz jvz self-assigned this Dec 29, 2023
@jvz jvz added this to the 2.23.0 milestone Dec 29, 2023
@jvz jvz added bug Incorrect, unexpected, or unintended behavior of existing code runtime Specific to the runtime environment labels Dec 29, 2023
@jvz
Copy link
Member

jvz commented Dec 29, 2023

No problem keeping support for SecurityManager. Just requires additional refactoring ;)

@jvz
Copy link
Member

jvz commented Dec 29, 2023

This bug appears to be yet another sign that Android's version of Java 8 is non-standard. As the security manager stuff is only relevant when one is installed, I think this class just needs to skip the AccessController::doPrivileged wrapper when no security manager is installed.

@tcmot
Copy link

tcmot commented Dec 30, 2023

Hi, all

Android SDK is not fully compatible with Openjdk. If you want Android to use log4j2, you need to remove the Openjdk API used in log4j2, which does not exist in Android SDK.

@jvz jvz closed this as completed in #2138 Jan 1, 2024
jvz added a commit that referenced this issue Jan 1, 2024
This fixes issue #2129 where `AccessController::doPrivileged` is
needlessly invoked when no `SecurityManager` is installed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect, unexpected, or unintended behavior of existing code runtime Specific to the runtime environment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants