From 8de4655a91264da50164a62efa4342187d843f1d Mon Sep 17 00:00:00 2001 From: Aaron Ogburn Date: Tue, 11 Oct 2022 15:34:13 -0400 Subject: [PATCH 1/3] [JBEE-258] FactoryFinderCache does not properly handle comments in service --- .../jboss/el/cache/FactoryFinderCache.java | 31 +++++++------------ 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java b/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java index 22e434d..eacd463 100644 --- a/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java +++ b/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java @@ -25,9 +25,11 @@ import java.io.BufferedReader; import java.io.InputStream; import java.io.InputStreamReader; +import java.lang.Class; import java.util.Iterator; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.ServiceLoader; /** * @author Stuart Douglas @@ -82,30 +84,21 @@ public static String loadImplementationClassName(final String factoryId, final C } } - String serviceId = "META-INF/services/" + factoryId; + Object result = null; // try to find services in CLASSPATH try { - InputStream is = null; - if (classLoader == null) { - is = ClassLoader.getSystemResourceAsStream(serviceId); - } else { - is = classLoader.getResourceAsStream(serviceId); + Class factoryClass = Class.forName(factoryId); + ServiceLoader serviceLoader = ServiceLoader.load(factoryClass, classLoader); + Iterator iter = serviceLoader.iterator(); + while (result == null && iter.hasNext()) { + result = iter.next(); } - if (is != null) { - BufferedReader rd = - new BufferedReader(new InputStreamReader(is, "UTF-8")); - - String factoryClassName = rd.readLine(); - rd.close(); - - if (factoryClassName != null && - !"".equals(factoryClassName)) { - if (classCache != null) { - classCache.put(new CacheKey(classLoader, factoryId), factoryClassName); - } - return factoryClassName; + if (result != null) { + if (classCache != null) { + classCache.put(new CacheKey(classLoader, factoryId), result.getClass().getName()); } + return result.getClass().getName(); } } catch (Exception ex) { } From 6361dc3432aa43a49ba628ee5e4d172a0d594f48 Mon Sep 17 00:00:00 2001 From: Tomas Hofman Date: Thu, 5 Jan 2023 10:59:28 +0100 Subject: [PATCH 2/3] JBEE-258 Test case for service file containing a comment --- .../org/jboss/el/cache/FactoryFinderCache.java | 3 --- .../jboss/el/cache/FactoryFinderCacheTestCase.java | 14 ++++++++++++++ .../test/java/org/jboss/el/cache/TestService.java | 5 +++++ .../java/org/jboss/el/cache/TestServiceImpl.java | 9 +++++++++ .../services/org.jboss.el.cache.TestService | 2 ++ 5 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java create mode 100644 api/src/test/java/org/jboss/el/cache/TestService.java create mode 100644 api/src/test/java/org/jboss/el/cache/TestServiceImpl.java create mode 100644 api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService diff --git a/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java b/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java index eacd463..c188769 100644 --- a/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java +++ b/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java @@ -22,9 +22,6 @@ package org.jboss.el.cache; -import java.io.BufferedReader; -import java.io.InputStream; -import java.io.InputStreamReader; import java.lang.Class; import java.util.Iterator; import java.util.Map; diff --git a/api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java b/api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java new file mode 100644 index 0000000..1d6510b --- /dev/null +++ b/api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java @@ -0,0 +1,14 @@ +package org.jboss.el.cache; + +import org.junit.Assert; +import org.junit.Test; + +public class FactoryFinderCacheTestCase { + + @Test + public void testServiceFileWithComment() { + String className = FactoryFinderCache.loadImplementationClassName("org.jboss.el.cache.TestService", + this.getClass().getClassLoader()); + Assert.assertEquals("org.jboss.el.cache.TestServiceImpl", className); + } +} diff --git a/api/src/test/java/org/jboss/el/cache/TestService.java b/api/src/test/java/org/jboss/el/cache/TestService.java new file mode 100644 index 0000000..c0e7eca --- /dev/null +++ b/api/src/test/java/org/jboss/el/cache/TestService.java @@ -0,0 +1,5 @@ +package org.jboss.el.cache; + +public interface TestService { + void execute(); +} diff --git a/api/src/test/java/org/jboss/el/cache/TestServiceImpl.java b/api/src/test/java/org/jboss/el/cache/TestServiceImpl.java new file mode 100644 index 0000000..07771b4 --- /dev/null +++ b/api/src/test/java/org/jboss/el/cache/TestServiceImpl.java @@ -0,0 +1,9 @@ +package org.jboss.el.cache; + +public class TestServiceImpl implements TestService { + + @Override + public void execute() { + // pass + } +} diff --git a/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService b/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService new file mode 100644 index 0000000..4a1e754 --- /dev/null +++ b/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService @@ -0,0 +1,2 @@ +# Comment +org.jboss.el.cache.TestServiceImpl \ No newline at end of file From 8ca9d3b8cbc45ac0741d475ddbfae1bd56708dff Mon Sep 17 00:00:00 2001 From: Tomas Hofman Date: Fri, 6 Jan 2023 10:20:10 +0100 Subject: [PATCH 3/3] JBEE-258 Keep using class.getResourceAsStream() instead ServiceLoader.load() ...and sanitize the input read from the service file. --- .../jboss/el/cache/FactoryFinderCache.java | 60 +++++++++++++++---- .../el/cache/FactoryFinderCacheTestCase.java | 25 ++++++++ .../services/org.jboss.el.cache.TestService | 1 - .../org.jboss.el.cache.TestService-comment | 2 + .../org.jboss.el.cache.TestService-empty | 0 5 files changed, 75 insertions(+), 13 deletions(-) create mode 100644 api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService-comment create mode 100644 api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService-empty diff --git a/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java b/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java index c188769..51b1e09 100644 --- a/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java +++ b/api/src/main/java/org/jboss/el/cache/FactoryFinderCache.java @@ -22,11 +22,12 @@ package org.jboss.el.cache; -import java.lang.Class; +import java.io.BufferedReader; +import java.io.InputStream; +import java.io.InputStreamReader; import java.util.Iterator; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import java.util.ServiceLoader; /** * @author Stuart Douglas @@ -81,21 +82,34 @@ public static String loadImplementationClassName(final String factoryId, final C } } - Object result = null; + String serviceId = "META-INF/services/" + factoryId; // try to find services in CLASSPATH try { - Class factoryClass = Class.forName(factoryId); - ServiceLoader serviceLoader = ServiceLoader.load(factoryClass, classLoader); - Iterator iter = serviceLoader.iterator(); - while (result == null && iter.hasNext()) { - result = iter.next(); + InputStream is = null; + if (classLoader == null) { + is = ClassLoader.getSystemResourceAsStream(serviceId); + } else { + is = classLoader.getResourceAsStream(serviceId); } - if (result != null) { - if (classCache != null) { - classCache.put(new CacheKey(classLoader, factoryId), result.getClass().getName()); + if (is != null) { + BufferedReader rd = + new BufferedReader(new InputStreamReader(is, "UTF-8")); + + String factoryClassName; + do { + factoryClassName = sanitize(rd.readLine()); + } while (factoryClassName != null && factoryClassName.isEmpty()); + + rd.close(); + + if (factoryClassName != null && + !"".equals(factoryClassName)) { + if (classCache != null) { + classCache.put(new CacheKey(classLoader, factoryId), factoryClassName); + } + return factoryClassName; } - return result.getClass().getName(); } } catch (Exception ex) { } @@ -105,6 +119,28 @@ public static String loadImplementationClassName(final String factoryId, final C return null; } + static String sanitize(String line) { + if (line == null) { + return null; + } + + // strip comment + int idx = line.indexOf('#'); + if (idx >= 0) { + line = line.substring(0, idx); + } + + // strip spaces and tabs + while (line.startsWith(" ") || line.startsWith("\t")) { + line = line.substring(1); + } + while (line.endsWith(" ") || line.endsWith("\t")) { + line = line.substring(0, line.length() - 1); + } + + return line; + } + private static class CacheKey { private final ClassLoader loader; private final String className; diff --git a/api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java b/api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java index 1d6510b..40c1646 100644 --- a/api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java +++ b/api/src/test/java/org/jboss/el/cache/FactoryFinderCacheTestCase.java @@ -7,8 +7,33 @@ public class FactoryFinderCacheTestCase { @Test public void testServiceFileWithComment() { + String className = FactoryFinderCache.loadImplementationClassName("org.jboss.el.cache.TestService-comment", + this.getClass().getClassLoader()); + Assert.assertEquals("org.jboss.el.cache.TestServiceImpl", className); + } + + @Test + public void testEmptyServiceFile() { + String className = FactoryFinderCache.loadImplementationClassName("org.jboss.el.cache.TestService-empty", + this.getClass().getClassLoader()); + Assert.assertNull(className); + } + + @Test + public void testServiceFile() { String className = FactoryFinderCache.loadImplementationClassName("org.jboss.el.cache.TestService", this.getClass().getClassLoader()); Assert.assertEquals("org.jboss.el.cache.TestServiceImpl", className); } + + @Test + public void testSanitizeMethod() { + Assert.assertEquals("ClassName", FactoryFinderCache.sanitize("ClassName")); + Assert.assertEquals("ClassName", FactoryFinderCache.sanitize(" ClassName ")); + Assert.assertEquals("ClassName", FactoryFinderCache.sanitize("\tClassName\t")); + Assert.assertEquals("ClassName", FactoryFinderCache.sanitize(" \tClassName\t # comment...")); + Assert.assertEquals("", FactoryFinderCache.sanitize("# only comment...")); + Assert.assertEquals("", FactoryFinderCache.sanitize("")); + Assert.assertNull(FactoryFinderCache.sanitize(null)); + } } diff --git a/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService b/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService index 4a1e754..06cde22 100644 --- a/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService +++ b/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService @@ -1,2 +1 @@ -# Comment org.jboss.el.cache.TestServiceImpl \ No newline at end of file diff --git a/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService-comment b/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService-comment new file mode 100644 index 0000000..4a1e754 --- /dev/null +++ b/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService-comment @@ -0,0 +1,2 @@ +# Comment +org.jboss.el.cache.TestServiceImpl \ No newline at end of file diff --git a/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService-empty b/api/src/test/resources/META-INF/services/org.jboss.el.cache.TestService-empty new file mode 100644 index 0000000..e69de29