Skip to content

Commit 2fe1f53

Browse files
committed
Improve FactoryFinder Validation
Also modernizes with generics and adds improved testing
1 parent 1bac615 commit 2fe1f53

File tree

20 files changed

+1065
-98
lines changed

20 files changed

+1065
-98
lines changed

activemq-broker/src/main/java/org/apache/activemq/broker/BrokerFactory.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,15 @@
3131
*/
3232
public final class BrokerFactory {
3333

34-
private static final FactoryFinder BROKER_FACTORY_HANDLER_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/broker/");
34+
private static final FactoryFinder<BrokerFactoryHandler> BROKER_FACTORY_HANDLER_FINDER
35+
= new FactoryFinder<>("META-INF/services/org/apache/activemq/broker/", BrokerFactoryHandler.class);
3536

3637
private BrokerFactory() {
3738
}
3839

3940
public static BrokerFactoryHandler createBrokerFactoryHandler(String type) throws IOException {
4041
try {
41-
return (BrokerFactoryHandler)BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
42+
return BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
4243
} catch (Throwable e) {
4344
throw IOExceptionSupport.create("Could not load " + type + " factory:" + e, e);
4445
}

activemq-broker/src/main/java/org/apache/activemq/broker/region/group/GroupFactoryFinder.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@
2525
import org.apache.activemq.util.URISupport;
2626

2727
public class GroupFactoryFinder {
28-
private static final FactoryFinder GROUP_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/groups/");
28+
private static final FactoryFinder<MessageGroupMapFactory> GROUP_FACTORY_FINDER =
29+
new FactoryFinder<>("META-INF/services/org/apache/activemq/groups/", MessageGroupMapFactory.class);
2930

3031
private GroupFactoryFinder() {
3132
}
@@ -40,7 +41,7 @@ public static MessageGroupMapFactory createMessageGroupMapFactory(String type) t
4041
factoryType = factoryType.substring(0,p);
4142
properties = URISupport.parseQuery(propertiesString);
4243
}
43-
MessageGroupMapFactory result = (MessageGroupMapFactory)GROUP_FACTORY_FINDER.newInstance(factoryType);
44+
MessageGroupMapFactory result = GROUP_FACTORY_FINDER.newInstance(factoryType);
4445
if (properties != null && result != null){
4546
IntrospectionSupport.setProperties(result,properties);
4647
}

activemq-broker/src/main/java/org/apache/activemq/transport/auto/AutoTcpTransportServer.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,17 @@ public class AutoTcpTransportServer extends TcpTransportServer {
8080
protected int maxConnectionThreadPoolSize = Integer.MAX_VALUE;
8181
protected int protocolDetectionTimeOut = 30000;
8282

83-
private static final FactoryFinder TRANSPORT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/");
83+
private static final FactoryFinder<TransportFactory> TRANSPORT_FACTORY_FINDER =
84+
new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/", TransportFactory.class);
8485
private final ConcurrentMap<String, TransportFactory> transportFactories = new ConcurrentHashMap<String, TransportFactory>();
8586

86-
private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/wireformat/");
87+
private static final FactoryFinder<WireFormatFactory> WIREFORMAT_FACTORY_FINDER =
88+
new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class);
8789

8890
public WireFormatFactory findWireFormatFactory(String scheme, Map<String, Map<String, Object>> options) throws IOException {
8991
WireFormatFactory wff = null;
9092
try {
91-
wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
93+
wff = WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
9294
if (options != null) {
9395
final Map<String, Object> wfOptions = new HashMap<>();
9496
if (options.get(AutoTransportUtils.ALL) != null) {
@@ -117,7 +119,7 @@ public TransportFactory findTransportFactory(String scheme, Map<String, ?> optio
117119
if (tf == null) {
118120
// Try to load if from a META-INF property.
119121
try {
120-
tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme);
122+
tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme);
121123
if (options != null) {
122124
IntrospectionSupport.setProperties(tf, options);
123125
}

activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,15 @@ protected void unregister(long bundleId) {
172172
// ================================================================
173173

174174
@Override
175-
public Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
175+
public Object create(String path)
176+
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
177+
throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls");
178+
}
179+
180+
@SuppressWarnings("unchecked")
181+
@Override
182+
public <T> T create(String path, Class<T> requiredType, Set<Class<? extends T>> allowedImpls)
183+
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
176184
Class<?> clazz = serviceCache.get(path);
177185
if (clazz == null) {
178186
StringBuilder warnings = new StringBuilder();
@@ -199,6 +207,10 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
199207
continue;
200208
}
201209

210+
// no reason to cache if invalid so validate before caching
211+
// the class inside of serviceCache
212+
FactoryFinder.validateClass(clazz, requiredType, allowedImpls);
213+
202214
// Yay.. the class was found. Now cache it.
203215
serviceCache.put(path, clazz);
204216
wrapper.cachedServices.add(path);
@@ -214,10 +226,17 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
214226
}
215227
throw new IOException(msg);
216228
}
229+
} else {
230+
// Validate again (even for previously cached classes) in case
231+
// a path is re-used with a different requiredType.
232+
// This object factory is shared by all factory finder instances, so it would be
233+
// possible (although probably a mistake) to use the same
234+
// path again with a different requiredType in a different FactoryFinder
235+
FactoryFinder.validateClass(clazz, requiredType, allowedImpls);
217236
}
218237

219238
try {
220-
return clazz.getConstructor().newInstance();
239+
return (T) clazz.getConstructor().newInstance();
221240
} catch (InvocationTargetException | NoSuchMethodException e) {
222241
throw new InstantiationException(e.getMessage());
223242
}

activemq-client/src/main/java/org/apache/activemq/transport/TransportFactory.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,10 @@
3636

3737
public abstract class TransportFactory {
3838

39-
private static final FactoryFinder TRANSPORT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/");
40-
private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/wireformat/");
39+
private static final FactoryFinder<TransportFactory> TRANSPORT_FACTORY_FINDER
40+
= new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/", TransportFactory.class);
41+
private static final FactoryFinder<WireFormatFactory> WIREFORMAT_FACTORY_FINDER
42+
= new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class);
4143
private static final ConcurrentMap<String, TransportFactory> TRANSPORT_FACTORYS = new ConcurrentHashMap<String, TransportFactory>();
4244

4345
private static final String WRITE_TIMEOUT_FILTER = "soWriteTimeout";
@@ -179,7 +181,7 @@ public static TransportFactory findTransportFactory(URI location) throws IOExcep
179181
if (tf == null) {
180182
// Try to load if from a META-INF property.
181183
try {
182-
tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme);
184+
tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme);
183185
TRANSPORT_FACTORYS.put(scheme, tf);
184186
} catch (Throwable e) {
185187
throw IOExceptionSupport.create("Transport scheme NOT recognized: [" + scheme + "]", e);
@@ -201,7 +203,7 @@ protected WireFormatFactory createWireFormatFactory(Map<String, String> options)
201203
}
202204

203205
try {
204-
WireFormatFactory wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
206+
WireFormatFactory wff = WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
205207
IntrospectionSupport.setProperties(wff, options, "wireFormat.");
206208
return wff;
207209
} catch (Throwable e) {

activemq-client/src/main/java/org/apache/activemq/transport/discovery/DiscoveryAgentFactory.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@
2626

2727
public abstract class DiscoveryAgentFactory {
2828

29-
private static final FactoryFinder DISCOVERY_AGENT_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/discoveryagent/");
29+
private static final FactoryFinder<DiscoveryAgentFactory> DISCOVERY_AGENT_FINDER =
30+
new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/discoveryagent/", DiscoveryAgentFactory.class);
3031
private static final ConcurrentMap<String, DiscoveryAgentFactory> DISCOVERY_AGENT_FACTORYS = new ConcurrentHashMap<String, DiscoveryAgentFactory>();
3132

3233
/**
@@ -43,7 +44,7 @@ private static DiscoveryAgentFactory findDiscoveryAgentFactory(URI uri) throws I
4344
if (daf == null) {
4445
// Try to load if from a META-INF property.
4546
try {
46-
daf = (DiscoveryAgentFactory)DISCOVERY_AGENT_FINDER.newInstance(scheme);
47+
daf = DISCOVERY_AGENT_FINDER.newInstance(scheme);
4748
DISCOVERY_AGENT_FACTORYS.put(scheme, daf);
4849
} catch (Throwable e) {
4950
throw IOExceptionSupport.create("DiscoveryAgent scheme NOT recognized: [" + scheme + "]", e);

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

Lines changed: 135 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,22 @@
2020
import java.io.IOException;
2121
import java.io.InputStream;
2222
import java.lang.reflect.InvocationTargetException;
23+
import java.net.UnknownServiceException;
24+
import java.nio.file.Path;
25+
import java.security.Security;
26+
import java.util.Arrays;
27+
import java.util.Objects;
28+
import java.util.Optional;
2329
import java.util.Properties;
30+
import java.util.Set;
2431
import java.util.concurrent.ConcurrentHashMap;
2532
import java.util.concurrent.ConcurrentMap;
33+
import java.util.stream.Collectors;
2634

2735
/**
2836
*
2937
*/
30-
public class FactoryFinder {
38+
public class FactoryFinder<T> {
3139

3240
/**
3341
* The strategy that the FactoryFinder uses to find load and instantiate Objects
@@ -44,51 +52,54 @@ public interface ObjectFactory {
4452
* @param path the full service path
4553
* @return
4654
*/
47-
public Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException;
55+
Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException;
4856

57+
@SuppressWarnings("unchecked")
58+
default <T> T create(String path, Class<T> requiredType, Set<Class<? extends T>> allowedImpls)
59+
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
60+
return (T) create(path);
61+
}
4962
}
5063

5164
/**
5265
* The default implementation of Object factory which works well in standalone applications.
5366
*/
5467
protected static class StandaloneObjectFactory implements ObjectFactory {
55-
final ConcurrentMap<String, Class> classMap = new ConcurrentHashMap<String, Class>();
68+
final ConcurrentMap<String, Class<?>> classMap = new ConcurrentHashMap<>();
5669

5770
@Override
58-
public Object create(final String path) throws InstantiationException, IllegalAccessException, ClassNotFoundException, IOException {
59-
Class clazz = classMap.get(path);
71+
public Object create(final String path)
72+
throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
73+
throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls");
74+
}
75+
76+
@SuppressWarnings("unchecked")
77+
@Override
78+
public <T> T create(String path, Class<T> requiredType, Set<Class<? extends T>> allowedImpls) throws InstantiationException, IllegalAccessException, ClassNotFoundException, IOException {
79+
Class<?> clazz = classMap.get(path);
6080
if (clazz == null) {
6181
clazz = loadClass(loadProperties(path));
82+
// no reason to cache if invalid so validate before caching
83+
validateClass(clazz, requiredType, allowedImpls);
6284
classMap.put(path, clazz);
85+
} else {
86+
// Validate again (even for previously cached classes) in case
87+
// a path is re-used with a different requiredType.
88+
// This object factory is static and shared by all factory finder instances by default,
89+
// so it would be possible (although probably a mistake) to use the same
90+
// path again with a different requiredType in a different FactoryFinder
91+
validateClass(clazz, requiredType, allowedImpls);
6392
}
64-
93+
6594
try {
66-
return clazz.getConstructor().newInstance();
95+
return (T) clazz.getConstructor().newInstance();
6796
} catch (NoSuchMethodException | InvocationTargetException e) {
6897
throw new InstantiationException(e.getMessage());
6998
}
7099
}
71100

72-
static public Class loadClass(Properties properties) throws ClassNotFoundException, IOException {
73-
74-
String className = properties.getProperty("class");
75-
if (className == null) {
76-
throw new IOException("Expected property is missing: class");
77-
}
78-
Class clazz = null;
79-
ClassLoader loader = Thread.currentThread().getContextClassLoader();
80-
if (loader != null) {
81-
try {
82-
clazz = loader.loadClass(className);
83-
} catch (ClassNotFoundException e) {
84-
// ignore
85-
}
86-
}
87-
if (clazz == null) {
88-
clazz = FactoryFinder.class.getClassLoader().loadClass(className);
89-
}
90-
91-
return clazz;
101+
static Class<?> loadClass(Properties properties) throws ClassNotFoundException, IOException {
102+
return FactoryFinder.loadClass(properties.getProperty("class"));
92103
}
93104

94105
static public Properties loadProperties(String uri) throws IOException {
@@ -138,21 +149,114 @@ public static void setObjectFactory(ObjectFactory objectFactory) {
138149
// Instance methods and properties
139150
// ================================================================
140151
private final String path;
152+
private final Class<T> requiredType;
153+
private final Set<Class<? extends T>> allowedImpls;
154+
155+
public FactoryFinder(String path, Class<T> requiredType) {
156+
this(path, requiredType, null);
157+
}
141158

142-
public FactoryFinder(String path) {
143-
this.path = path;
159+
public FactoryFinder(String path, Class<T> requiredType, String allowedImpls) {
160+
this.path = Objects.requireNonNull(path);
161+
this.requiredType = Objects.requireNonNull(requiredType);
162+
this.allowedImpls = loadAllowedImpls(requiredType, allowedImpls);
144163
}
145164

165+
@SuppressWarnings("unchecked")
166+
private static <T> Set<Class<? extends T>> loadAllowedImpls(Class<T> requiredType, String allowedImpls) {
167+
// If allowedImpls is either null or an asterisk (allow all wild card) then set to null so we don't filter
168+
// If allowedImpls is only an empty string we return an empty set meaning allow none
169+
// Otherwise split/trim all values
170+
return allowedImpls != null && !allowedImpls.equals("*") ?
171+
Arrays.stream(allowedImpls.split("\\s*,\\s*"))
172+
.filter(s -> !s.isEmpty())
173+
.map(s -> {
174+
try {
175+
final Class<?> clazz = FactoryFinder.loadClass(s);
176+
if (!requiredType.isAssignableFrom(clazz)) {
177+
throw new IllegalArgumentException(
178+
"Class " + clazz + " is not assignable to " + requiredType);
179+
}
180+
return (Class<? extends T>)clazz;
181+
} catch (ClassNotFoundException | IOException e) {
182+
throw new IllegalArgumentException(e);
183+
}
184+
}).collect(Collectors.toUnmodifiableSet()) : null;
185+
}
186+
187+
146188
/**
147189
* Creates a new instance of the given key
148190
*
149191
* @param key is the key to add to the path to find a text file containing
150192
* the factory name
151193
* @return a newly created instance
152194
*/
153-
public Object newInstance(String key) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
154-
return objectFactory.create(path+key);
195+
public T newInstance(String key) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException {
196+
return objectFactory.create(resolvePath(key), requiredType, allowedImpls);
197+
}
198+
199+
Set<Class<? extends T>> getAllowedImpls() {
200+
return allowedImpls;
155201
}
156202

203+
Class<T> getRequiredType() {
204+
return requiredType;
205+
}
206+
207+
private String resolvePath(final String key) throws InstantiationException {
208+
// Normalize the base path with the given key. This
209+
// will resolve/remove any relative ".." sections of the path.
210+
// Example: "/dir1/dir2/dir3/../file" becomes "/dir1/dir2/file"
211+
final Path resolvedPath = Path.of(path).resolve(key).normalize();
212+
213+
// Validate the resoled path is still within the original defined
214+
// root path and throw an error of it is not.
215+
if (!resolvedPath.startsWith(path)) {
216+
throw new InstantiationException("Provided key escapes the FactoryFinder configured directory");
217+
}
218+
219+
return resolvedPath.toString();
220+
}
221+
222+
public static String buildAllowedImpls(Class<?>...classes) {
223+
return Arrays.stream(Objects.requireNonNull(classes, "List of allowed classes may not be null"))
224+
.map(Class::getName).collect(Collectors.joining(","));
225+
}
226+
227+
public static <T> void validateClass(Class<?> clazz, Class<T> requiredType,
228+
Set<Class<? extends T>> allowedImpls) throws InstantiationException {
229+
// Validate the loaded class is an allowed impl
230+
if (allowedImpls != null && !allowedImpls.contains(clazz)) {
231+
throw new InstantiationException("Class " + clazz + " is not an allowed implementation "
232+
+ "of type " + requiredType);
233+
}
234+
// Validate the loaded class is a subclass of the right type
235+
// The allowedImpls may not be used so also check requiredType. Even if set
236+
// generics can be erased and this is an extra safety check
237+
if (!requiredType.isAssignableFrom(clazz)) {
238+
throw new InstantiationException("Class " + clazz + " is not assignable to " + requiredType);
239+
}
240+
}
241+
242+
static Class<?> loadClass(String className) throws ClassNotFoundException, IOException {
243+
if (className == null) {
244+
throw new IOException("Expected property is missing: class");
245+
}
246+
Class<?> clazz = null;
247+
ClassLoader loader = Thread.currentThread().getContextClassLoader();
248+
if (loader != null) {
249+
try {
250+
clazz = loader.loadClass(className);
251+
} catch (ClassNotFoundException e) {
252+
// ignore
253+
}
254+
}
255+
if (clazz == null) {
256+
clazz = FactoryFinder.class.getClassLoader().loadClass(className);
257+
}
258+
259+
return clazz;
260+
}
157261

158262
}

0 commit comments

Comments
 (0)