ppkarwasz commented on code in PR #2138:
URL: https://github.com/apache/logging-log4j2/pull/2138#discussion_r1438430189
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java:
##########
@@ -143,22 +140,29 @@ private static boolean isChild(final ClassLoader loader1,
final ClassLoader load
* @return the current thread's ClassLoader, a fallback loader, or null if
no fallback can be determined
*/
public static ClassLoader getThreadContextClassLoader() {
- if (GET_CLASS_LOADER_DISABLED) {
- // we can at least get this class's ClassLoader regardless of
security context
- // however, if this is null, there's really no option left at this
point
- try {
- return getThisClassLoader();
- } catch (final SecurityException ignored) {
- return null;
- }
+ try {
+ return GET_CLASS_LOADER_DISABLED
+ ? getThisClassLoader()
+ : runActionInvolvingGetClassLoaderPermission(TCCL_GETTER);
+ } catch (final SecurityException ignored) {
+ return null;
}
- return AccessController.doPrivileged(TCCL_GETTER, null,
GET_CLASS_LOADER);
}
private static ClassLoader getThisClassLoader() {
return LoaderUtil.class.getClassLoader();
}
+ private static <T> T runActionInvolvingGetClassLoaderPermission(final
PrivilegedAction<T> action) {
+ return System.getSecurityManager() != null
+ ? runPrivilegedActionWithGetClassLoaderPermission(action)
+ : action.run();
+ }
+
+ private static <T> T runPrivilegedActionWithGetClassLoaderPermission(final
PrivilegedAction<T> action) {
+ return AccessController.doPrivileged(action, null, GET_CLASS_LOADER);
+ }
Review Comment:
```suggestion
private static <T> T
runPrivilegedActionWithGetClassLoaderPermission(final PrivilegedAction<T>
action) {
return AccessController.doPrivileged(action);
}
```
Now that we know that Android does not have the
`AccessController.doPrivileged(PrivilegedAction<T>, AccessControlContext,
Permission...)` method (cf. [Android
javadoc](https://developer.android.com/reference/java/security/AccessController)),
why don't we call `AccessController.doPrivileged(PrivilegedAction<T>)` method?
Sure, you took care to protect every call site with a check to
`System.getSecurityManager() != null`, but I don't understand what is the
advantage of calling the 3-param method? Even if the `action` lambda only
requires the "getClassLoader" permission, I don't see the problem of giving to
it **all** the permissions of the caller.
By eliminating the 3-param method from our codebase, we prevent some future
errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
