You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by pk...@apache.org on 2023/02/07 17:26:57 UTC
[logging-log4j2] branch master updated: [LOG4J2-3624] Protects `ServiceLoaderUtil` from unchecked exception
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
The following commit(s) were added to refs/heads/master by this push:
new 043042b1a7 [LOG4J2-3624] Protects `ServiceLoaderUtil` from unchecked exception
043042b1a7 is described below
commit 043042b1a731a880684f8e2e7dfe2355512f6537
Author: Piotr P. Karwasz <pi...@karwasz.org>
AuthorDate: Thu Feb 2 23:03:19 2023 +0100
[LOG4J2-3624] Protects `ServiceLoaderUtil` from unchecked exception
Contrary to the documentation `ServiceLoader` can also throw unchecked
exceptions such as `NoClassDefFoundError` or other `LinkageError`s. This
should not prevent `ServiceLoaderUtil` from returning those services,
which are not broken.
This PR logs all exceptions, but rethrows everything exception
`LinkageError` and `ServiceConfigurationError`: users expect exceptions
such as `SecurityException` to break Log4j2 API until an administrator
fixes them.
---
.../apache/logging/log4j/test/BetterService.java | 3 +-
.../org/apache/logging/log4j/test/Service.java | 3 +-
.../org/apache/logging/log4j/test/Service1.java | 3 +-
.../org/apache/logging/log4j/test/Service2.java | 3 +-
.../logging/log4j/util/ServiceLoaderUtilTest.java | 61 +++++++++++-----------
...=> org.apache.logging.log4j.test.BetterService} | 2 +-
...rvice => org.apache.logging.log4j.test.Service} | 11 ++--
.../logging/log4j/test/ForceLinkageError.class | 0
.../logging/log4j/util/ServiceLoaderUtil.java | 18 +++++--
...atch_unchecked_exception_from_ServiceLoader.xml | 27 ++++++++++
10 files changed, 85 insertions(+), 46 deletions(-)
diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/BetterService.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/BetterService.java
index ca8b893e0e..3cce43b4a3 100644
--- a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/BetterService.java
+++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/BetterService.java
@@ -14,8 +14,7 @@
* See the license for the specific language governing permissions and
* limitations under the license.
*/
-
package org.apache.logging.log4j.test;
public interface BetterService extends Service {
-}
\ No newline at end of file
+}
diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service.java
index 25b1b33681..bfc706dc1e 100644
--- a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service.java
+++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service.java
@@ -14,8 +14,7 @@
* See the license for the specific language governing permissions and
* limitations under the license.
*/
-
package org.apache.logging.log4j.test;
public interface Service {
-}
\ No newline at end of file
+}
diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service1.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service1.java
index 82c233b852..fb9942d0fb 100644
--- a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service1.java
+++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service1.java
@@ -14,8 +14,7 @@
* See the license for the specific language governing permissions and
* limitations under the license.
*/
-
package org.apache.logging.log4j.test;
public class Service1 implements Service {
-}
\ No newline at end of file
+}
diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service2.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service2.java
index c4a8c39b97..e25d348641 100644
--- a/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service2.java
+++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/test/Service2.java
@@ -14,8 +14,7 @@
* See the license for the specific language governing permissions and
* limitations under the license.
*/
-
package org.apache.logging.log4j.test;
public class Service2 implements BetterService {
-}
\ No newline at end of file
+}
diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/util/ServiceLoaderUtilTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/ServiceLoaderUtilTest.java
index ab8e9051d0..80c34ef957 100644
--- a/log4j-api-test/src/test/java/org/apache/logging/log4j/util/ServiceLoaderUtilTest.java
+++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/ServiceLoaderUtilTest.java
@@ -17,51 +17,52 @@
package org.apache.logging.log4j.util;
import java.lang.invoke.MethodHandles;
-import java.util.Collections;
+import java.util.ArrayList;
import java.util.List;
import java.util.ServiceConfigurationError;
import java.util.stream.Collectors;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.status.StatusData;
import org.apache.logging.log4j.test.BetterService;
+import org.apache.logging.log4j.test.ListStatusListener;
import org.apache.logging.log4j.test.Service;
+import org.apache.logging.log4j.test.junit.UsingStatusListener;
import org.junit.jupiter.api.Test;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.fail;
public class ServiceLoaderUtilTest {
@Test
public void testServiceResolution() {
- // Run only if we are a module
- if (ServiceLoaderUtil.class.getModule().isNamed()) {
- List<Object> services = Collections.emptyList();
- // Service from test module
- try {
- services = ServiceLoaderUtil.loadServices(Service.class, MethodHandles.lookup())
- .collect(Collectors.toList());
- } catch (ServiceConfigurationError e) {
- fail(e);
- }
- assertEquals(2, services.size(), "Service services");
- // BetterService from test module
+ final List<Object> services = new ArrayList<>();
+ assertDoesNotThrow(() -> ServiceLoaderUtil.loadServices(BetterService.class, MethodHandles.lookup(), false)
+ .forEach(services::add));
+ assertThat(services).hasSize(1);
services.clear();
- try {
- services = ServiceLoaderUtil.loadServices(BetterService.class, MethodHandles.lookup())
- .collect(Collectors.toList());
- } catch (ServiceConfigurationError e) {
- fail(e);
- }
- assertEquals(1, services.size(), "BetterService services");
- // PropertySource from org.apache.logging.log4j module from this module
- services.clear();
- try {
- services = ServiceLoaderUtil.loadServices(PropertySource.class, MethodHandles.lookup())
- .collect(Collectors.toList());
- } catch (ServiceConfigurationError e) {
- fail(e);
- }
- assertEquals(0, services.size(), "PropertySource services");
+ assertDoesNotThrow(() -> ServiceLoaderUtil.loadServices(PropertySource.class, MethodHandles.lookup(), false)
+ .forEach(services::add));
+ assertThat(services).hasSize(3);
}
+
+ @Test
+ @UsingStatusListener
+ public void testBrokenServiceFile(final ListStatusListener listener) {
+ final List<Service> services = new ArrayList<>();
+ assertDoesNotThrow(() -> ServiceLoaderUtil.loadServices(Service.class, MethodHandles.lookup(), false)
+ .forEach(services::add));
+ assertEquals(2, services.size());
+ // A warning for each broken service
+ final List<Throwable> errors = listener.findStatusData(Level.WARN).map(StatusData::getThrowable)
+ .collect(Collectors.toList());
+ assertThat(errors.stream().map(Throwable::getMessage))
+ .allMatch(message -> message.endsWith("invalid.Service not found")
+ || message.endsWith("org.apache.logging.log4j.Logger not a subtype")
+ || message.endsWith("Truncated class file"));
+ assertThat(errors.stream().<Class<?>>map(Throwable::getClass)).containsExactlyInAnyOrder(
+ ServiceConfigurationError.class, ServiceConfigurationError.class, ClassFormatError.class);
}
}
diff --git a/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.util.test.BetterService b/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.test.BetterService
similarity index 94%
rename from log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.util.test.BetterService
rename to log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.test.BetterService
index 69b68983a6..590e752c76 100644
--- a/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.util.test.BetterService
+++ b/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.test.BetterService
@@ -13,4 +13,4 @@
# See the license for the specific language governing permissions and
# limitations under the license.
-org.apache.logging.log4j.util.test.Service2
+org.apache.logging.log4j.test.Service2
diff --git a/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.util.test.Service b/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.test.Service
similarity index 74%
rename from log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.util.test.Service
rename to log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.test.Service
index 5fde1d5351..5b9b8918af 100644
--- a/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.util.test.Service
+++ b/log4j-api-test/src/test/resources/META-INF/services/org.apache.logging.log4j.test.Service
@@ -14,13 +14,18 @@
# limitations under the license.
# A correct entry
-org.apache.logging.log4j.util.test.Service1
+org.apache.logging.log4j.test.Service1
# Simulates a service retrieved from the server's classloader in a servlet environment.
-java.lang.String
+# The class exists but does not extend org.apache.logging.log4j.test.Service
+org.apache.logging.log4j.Logger
# Simulates a broken service.
+# Should cause a ClassNotFoundError
invalid.Service
+# Causes an exception not caught by ServiceLoader
+org.apache.logging.log4j.test.ForceLinkageError
+
# Another correct service
-org.apache.logging.log4j.util.test.Service2
+org.apache.logging.log4j.test.Service2
diff --git a/log4j-api-test/src/test/resources/org/apache/logging/log4j/test/ForceLinkageError.class b/log4j-api-test/src/test/resources/org/apache/logging/log4j/test/ForceLinkageError.class
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
index 99a24561ea..b9fae33b19 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
@@ -43,6 +43,8 @@ import org.apache.logging.log4j.status.StatusLogger;
@InternalApi
public final class ServiceLoaderUtil {
+ private static final int MAX_BROKEN_SERVICES = 8;
+
private ServiceLoaderUtil() {
}
@@ -146,14 +148,22 @@ public final class ServiceLoaderUtil {
@Override
public boolean tryAdvance(Consumer<? super S> action) {
- while (serviceIterator.hasNext()) {
+ int i = MAX_BROKEN_SERVICES;
+ while (i-- > 0) {
try {
- action.accept(serviceIterator.next());
- return true;
- } catch (ServiceConfigurationError e) {
+ if (serviceIterator.hasNext()) {
+ action.accept(serviceIterator.next());
+ return true;
+ }
+ } catch (ServiceConfigurationError | LinkageError e) {
+ if (logger != null) {
+ logger.warn("Unable to load service class for service {}", serviceName, e);
+ }
+ } catch (Throwable e) {
if (logger != null) {
logger.warn("Unable to load service class for service {}", serviceName, e);
}
+ throw e;
}
}
return false;
diff --git a/src/changelog/.2.x.x/LOG4J2-3624_Catch_unchecked_exception_from_ServiceLoader.xml b/src/changelog/.2.x.x/LOG4J2-3624_Catch_unchecked_exception_from_ServiceLoader.xml
new file mode 100644
index 0000000000..4fa628957b
--- /dev/null
+++ b/src/changelog/.2.x.x/LOG4J2-3624_Catch_unchecked_exception_from_ServiceLoader.xml
@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xmlns="http://logging.apache.org/log4j/changelog"
+ xsi:schemaLocation="http://logging.apache.org/log4j/changelog https://logging.apache.org/log4j/changelog-0.1.0.xsd"
+ type="fixed">
+ <issue id="LOG4J2-3624" link="https://issues.apache.org/jira/browse/LOG4J2-3624"/>
+ <author id="pkarwasz"/>
+ <description format="asciidoc">
+ Protects `ServiceLoaderUtil` from unchecked `ServiceLoader` exceptions.
+ </description>
+</entry>