Skip to content

Conversation

jvz
Copy link
Member

@jvz jvz commented Dec 29, 2023

This fixes issue #2129 where AccessController::doPrivileged is needlessly invoked when no SecurityManager is installed.

This fixes issue apache#2129 where `AccessController::doPrivileged` is
needlessly invoked when no `SecurityManager` is installed.
@jvz jvz requested review from ppkarwasz and vy December 29, 2023 20:56
@jvz jvz added bug Incorrect, unexpected, or unintended behavior of existing code runtime Specific to the runtime environment labels Dec 29, 2023
@jvz jvz added this to the 2.23.0 milestone Dec 29, 2023
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

@jvz,

Thank You for the fast fix!

Personally I think we should use as little SecurityManager code as possible, see my remark below.

@vy
Copy link
Member

vy commented Dec 29, 2023

@jvz, would you also mind adding tests too, please?

Turns out Android doesn't support Java 8 all that much.
@jvz jvz requested a review from ppkarwasz December 30, 2023 23:52
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

LGTM, although it would be nice to have an Android test.

static {
if (System.getSecurityManager() != null) {
boolean getClassLoaderDisabled;
static final RuntimePermission GET_CLASS_LOADER = new RuntimePermission("getClassLoader");
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I am mistaken, this is no longer necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used for checking if we have the ability to use the permission in full-permission mode to see if we should just fall back to the class loader of this class.

@jvz jvz merged commit 8b72076 into apache:2.x Jan 1, 2024
@jvz jvz deleted the issue-2129 branch January 1, 2024 20:00
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 this pull request may close these issues.

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

3 participants