Skip to content

Commit 9076906

Browse files
jgallimorejgallimore
committed
Fix FactoryFinder path resolution in Windows (apache#1831) (apache#1837)
This update ensures forward slashes are alow used for classpath resolution, even on all Windows versions. Also add extra validation to prevent path separators (slashes) in factory key names. Lastly added some more tests for validation This closes apache#1830 (cherry picked from commit bedb108)
1 parent e30ef41 commit 9076906

25 files changed

Lines changed: 1368 additions & 109 deletions

File tree

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,16 @@
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,
36+
null);
3537

3638
private BrokerFactory() {
3739
}
3840

3941
public static BrokerFactoryHandler createBrokerFactoryHandler(String type) throws IOException {
4042
try {
41-
return (BrokerFactoryHandler)BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
43+
return BROKER_FACTORY_HANDLER_FINDER.newInstance(type);
4244
} catch (Throwable e) {
4345
throw IOExceptionSupport.create("Could not load " + type + " factory:" + e, e);
4446
}

‎activemq-broker/src/main/java/org/apache/activemq/broker/jmx/BrokerView.java‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.io.File;
2020
import java.io.IOException;
2121
import java.net.URI;
22+
import java.net.URISyntaxException;
2223
import java.util.*;
2324
import java.util.concurrent.atomic.AtomicInteger;
2425

@@ -35,6 +36,7 @@
3536
import org.apache.activemq.command.*;
3637
import org.apache.activemq.network.NetworkConnector;
3738
import org.apache.activemq.util.BrokerSupport;
39+
import org.apache.activemq.util.URISupport;
3840
import org.slf4j.Logger;
3941
import org.slf4j.LoggerFactory;
4042

@@ -370,6 +372,8 @@ public ObjectName[] getDynamicDestinationProducers() {
370372

371373
@Override
372374
public String addConnector(String discoveryAddress) throws Exception {
375+
// Verify VM transport is not used
376+
validateAllowedUrl(discoveryAddress);
373377
TransportConnector connector = brokerService.addConnector(discoveryAddress);
374378
if (connector == null) {
375379
throw new NoSuchElementException("Not connector matched the given name: " + discoveryAddress);
@@ -380,6 +384,8 @@ public String addConnector(String discoveryAddress) throws Exception {
380384

381385
@Override
382386
public String addNetworkConnector(String discoveryAddress) throws Exception {
387+
// Verify VM transport is not used
388+
validateAllowedUrl(discoveryAddress);
383389
NetworkConnector connector = brokerService.addNetworkConnector(discoveryAddress);
384390
if (connector == null) {
385391
throw new NoSuchElementException("Not connector matched the given name: " + discoveryAddress);
@@ -541,4 +547,42 @@ private ConnectionContext getConnectionContext() {
541547

542548
return context;
543549
}
550+
551+
private static void validateAllowedUrl(String uriString) throws URISyntaxException {
552+
validateAllowedUri(new URI(uriString), 0);
553+
}
554+
555+
// Validate the URI does not contain VM transport
556+
private static void validateAllowedUri(URI uri, int depth) throws URISyntaxException {
557+
// Don't allow more than 5 nested URIs to prevent blowing the stack
558+
if (depth > 5) {
559+
throw new IllegalArgumentException("URI can't contain more than 5 nested composite URIs");
560+
}
561+
562+
// First check the main URI scheme
563+
validateAllowedScheme(uri.getScheme());
564+
565+
// If composite, iterate and check each of the composite URIs
566+
if (URISupport.isCompositeURI(uri)) {
567+
URISupport.CompositeData data = URISupport.parseComposite(uri);
568+
depth++;
569+
for (URI component : data.getComponents()) {
570+
// Each URI could be a nested composite URI so call validateAllowedUri()
571+
// to validate it. This check if composite first so we don't add to
572+
// the recursive stack depth if there's a lot of URIs that are not composite
573+
if (URISupport.isCompositeURI(uri)) {
574+
validateAllowedUri(component, depth);
575+
} else {
576+
validateAllowedScheme(uri.getScheme());
577+
}
578+
}
579+
}
580+
}
581+
582+
// We don't allow VM transport scheme to be used
583+
private static void validateAllowedScheme(String scheme) {
584+
if (scheme.equals("vm")) {
585+
throw new IllegalArgumentException("VM scheme is not allowed");
586+
}
587+
}
544588
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
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,
30+
null);
2931

3032
private GroupFactoryFinder() {
3133
}
@@ -40,7 +42,7 @@ public static MessageGroupMapFactory createMessageGroupMapFactory(String type) t
4042
factoryType = factoryType.substring(0,p);
4143
properties = URISupport.parseQuery(propertiesString);
4244
}
43-
MessageGroupMapFactory result = (MessageGroupMapFactory)GROUP_FACTORY_FINDER.newInstance(factoryType);
45+
MessageGroupMapFactory result = GROUP_FACTORY_FINDER.newInstance(factoryType);
4446
if (properties != null && result != null){
4547
IntrospectionSupport.setProperties(result,properties);
4648
}

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,19 @@ 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,
85+
null);
8486
private final ConcurrentMap<String, TransportFactory> transportFactories = new ConcurrentHashMap<String, TransportFactory>();
8587

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

8892
public WireFormatFactory findWireFormatFactory(String scheme, Map<String, Map<String, Object>> options) throws IOException {
8993
WireFormatFactory wff = null;
9094
try {
91-
wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
95+
wff = WIREFORMAT_FACTORY_FINDER.newInstance(scheme);
9296
if (options != null) {
9397
final Map<String, Object> wfOptions = new HashMap<>();
9498
if (options.get(AutoTransportUtils.ALL) != null) {
@@ -117,7 +121,7 @@ public TransportFactory findTransportFactory(String scheme, Map<String, ?> optio
117121
if (tf == null) {
118122
// Try to load if from a META-INF property.
119123
try {
120-
tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme);
124+
tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme);
121125
if (options != null) {
122126
IntrospectionSupport.setProperties(tf, options);
123127
}

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

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.io.IOException;
2323
import java.io.InputStream;
2424
import java.io.InputStreamReader;
25+
import java.lang.reflect.InvocationTargetException;
2526
import java.net.URL;
2627
import java.util.ArrayList;
2728
import java.util.HashSet;
@@ -171,7 +172,15 @@ protected void unregister(long bundleId) {
171172
// ================================================================
172173

173174
@Override
174-
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 {
175184
Class<?> clazz = serviceCache.get(path);
176185
if (clazz == null) {
177186
StringBuffer warnings = new StringBuffer();
@@ -198,6 +207,10 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
198207
continue;
199208
}
200209

210+
// no reason to cache if invalid so validate before caching
211+
// the class inside of serviceCache
212+
FactoryFinder.validateClass(clazz, requiredType, allowedImpls);
213+
201214
// Yay.. the class was found. Now cache it.
202215
serviceCache.put(path, clazz);
203216
wrapper.cachedServices.add(path);
@@ -213,8 +226,19 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx
213226
}
214227
throw new IOException(msg);
215228
}
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);
236+
}
237+
try {
238+
return (T) clazz.getConstructor().newInstance();
239+
} catch (InvocationTargetException | NoSuchMethodException e) {
240+
throw new InstantiationException(e.getMessage());
216241
}
217-
return clazz.newInstance();
218242
}
219243

220244
// ================================================================

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,12 @@
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+
null);
42+
private static final FactoryFinder<WireFormatFactory> WIREFORMAT_FACTORY_FINDER
43+
= new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class,
44+
null);
4145
private static final ConcurrentMap<String, TransportFactory> TRANSPORT_FACTORYS = new ConcurrentHashMap<String, TransportFactory>();
4246

4347
private static final String WRITE_TIMEOUT_FILTER = "soWriteTimeout";
@@ -179,7 +183,7 @@ public static TransportFactory findTransportFactory(URI location) throws IOExcep
179183
if (tf == null) {
180184
// Try to load if from a META-INF property.
181185
try {
182-
tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme);
186+
tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme);
183187
TRANSPORT_FACTORYS.put(scheme, tf);
184188
} catch (Throwable e) {
185189
throw IOExceptionSupport.create("Transport scheme NOT recognized: [" + scheme + "]", e);
@@ -201,7 +205,7 @@ protected WireFormatFactory createWireFormatFactory(Map<String, String> options)
201205
}
202206

203207
try {
204-
WireFormatFactory wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
208+
WireFormatFactory wff = WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat);
205209
IntrospectionSupport.setProperties(wff, options, "wireFormat.");
206210
return wff;
207211
} catch (Throwable e) {

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
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/",
31+
DiscoveryAgentFactory.class, null);
3032
private static final ConcurrentMap<String, DiscoveryAgentFactory> DISCOVERY_AGENT_FACTORYS = new ConcurrentHashMap<String, DiscoveryAgentFactory>();
3133

3234
/**
@@ -43,7 +45,7 @@ private static DiscoveryAgentFactory findDiscoveryAgentFactory(URI uri) throws I
4345
if (daf == null) {
4446
// Try to load if from a META-INF property.
4547
try {
46-
daf = (DiscoveryAgentFactory)DISCOVERY_AGENT_FINDER.newInstance(scheme);
48+
daf = DISCOVERY_AGENT_FINDER.newInstance(scheme);
4749
DISCOVERY_AGENT_FACTORYS.put(scheme, daf);
4850
} catch (Throwable e) {
4951
throw IOExceptionSupport.create("DiscoveryAgent scheme NOT recognized: [" + scheme + "]", e);

0 commit comments

Comments
 (0)