You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/08/21 11:47:12 UTC
[GitHub] [logging-log4j2] boris-unckel opened a new pull request, #1007: LOG4J2-3579 Add privileged execution for ServiceLoading
boris-unckel opened a new pull request, #1007:
URL: https://github.com/apache/logging-log4j2/pull/1007
Add privileged actions on caller side
Fixes https://issues.apache.org/jira/browse/LOG4J2-3579
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1007: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #1007:
URL: https://github.com/apache/logging-log4j2/pull/1007#discussion_r951767519
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -446,22 +447,36 @@ private static class Environment {
private final Map<List<CharSequence>, String> tokenized = new ConcurrentHashMap<>();
private Environment(final PropertySource propertySource) {
- PropertyFilePropertySource sysProps = new PropertyFilePropertySource(LOG4J_SYSTEM_PROPERTIES_FILE_NAME, false);
- try {
- sysProps.forEach((key, value) -> {
- if (System.getProperty(key) == null) {
- System.setProperty(key, value);
- }
- });
- } catch (SecurityException ex) {
- // Access to System Properties is restricted so just skip it.
- }
+ PropertyFilePropertySource sysProps = new PropertyFilePropertySource(LOG4J_SYSTEM_PROPERTIES_FILE_NAME,
+ false);
+ sysProps.forEach((key, value) -> {
+ if (System.getProperty(key) == null) {
+ System.setProperty(key, value);
+ }
+ });
sources.add(propertySource);
+ sources.add(new SystemPropertiesPropertySource());
+ sources.add(new EnvironmentPropertySource());
Review Comment:
I wouldn't hard code these property sources. Isn't looking up services in the same protection domain always allowed?
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/EnvironmentPropertySource.java:
##########
@@ -38,21 +38,11 @@ public int getPriority() {
return DEFAULT_PRIORITY;
}
- private void logException(SecurityException e) {
- // There is no status logger yet.
- LowLevelLogUtil.logException(
- "The system environment variables are not available to Log4j due to security restrictions: " + e, e);
- }
-
@Override
public void forEach(final BiConsumer<String, String> action) {
final Map<String, String> getenv;
- try {
- getenv = System.getenv();
- } catch (final SecurityException e) {
- logException(e);
- return;
- }
+ getenv = System.getenv();
+
Review Comment:
`forEach` is an optional method for a property source: Log4j2 does **not** need access to all environment variables, but if it does, it can cache the variable values.
I would leave the `catch`, so that Log4j2 users do not need to give the `getenv.*` permission to `log4j-api`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java:
##########
@@ -39,17 +39,8 @@ public int getPriority() {
@Override
public void forEach(final BiConsumer<String, String> action) {
- Properties properties;
- try {
- properties = System.getProperties();
- } catch (final SecurityException e) {
- // (1) There is no status logger.
- // (2) LowLevelLogUtil also consults system properties ("line.separator") to
- // open a BufferedWriter, so this may fail as well. Just having a hard reference
- // in this code to LowLevelLogUtil would cause a problem.
- // (3) We could log to System.err (nah) or just be quiet as we do now.
- return;
- }
Review Comment:
This is not fatal, the catch&ignore is fine.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/EnvironmentPropertySource.java:
##########
@@ -75,32 +65,17 @@ public CharSequence getNormalForm(final Iterable<? extends CharSequence> tokens)
@Override
public Collection<String> getPropertyNames() {
- try {
return System.getenv().keySet();
- } catch (final SecurityException e) {
- logException(e);
- return PropertySource.super.getPropertyNames();
- }
Review Comment:
Same as above: the `getPropertyNames` is used for caching, but is not necessary for Log4j2.
I would leave the `catch`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java:
##########
@@ -71,20 +62,12 @@ public CharSequence getNormalForm(final Iterable<? extends CharSequence> tokens)
@Override
public Collection<String> getPropertyNames() {
- try {
return System.getProperties().stringPropertyNames();
- } catch (final SecurityException e) {
- return PropertySource.super.getPropertyNames();
- }
Review Comment:
Also non fatal.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] boris-unckel commented on pull request #1007: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
boris-unckel commented on PR #1007:
URL: https://github.com/apache/logging-log4j2/pull/1007#issuecomment-1223667217
Closed, solution is done with https://github.com/apache/logging-log4j2/pull/1008
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jamezp commented on pull request #1007: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
jamezp commented on PR #1007:
URL: https://github.com/apache/logging-log4j2/pull/1007#issuecomment-1222462944
I prefer this approach as opposed to #1006.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] boris-unckel closed pull request #1007: LOG4J2-3579 Add privileged execution for ServiceLoading
Posted by GitBox <gi...@apache.org>.
boris-unckel closed pull request #1007: LOG4J2-3579 Add privileged execution for ServiceLoading
URL: https://github.com/apache/logging-log4j2/pull/1007
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org