Skip to content

Commit efda86d

Browse files
sbordetsbordet
committed
Fixes COMETD-441 (Race condition in AbstractService for non-public
methods/classes). Also fixed ServerAnnotationProcessor and ClientAnnotationProcessor, along with all service classes used in tests.
1 parent f6ac212 commit efda86d

File tree

18 files changed

+1267
-934
lines changed

18 files changed

+1267
-934
lines changed

‎cometd-java/cometd-java-annotations/src/main/java/org/cometd/annotation/ClientAnnotationProcessor.java‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.lang.reflect.Field;
2020
import java.lang.reflect.InvocationTargetException;
2121
import java.lang.reflect.Method;
22+
import java.lang.reflect.Modifier;
2223
import java.util.Arrays;
2324
import java.util.List;
2425
import java.util.concurrent.ConcurrentHashMap;
@@ -109,6 +110,9 @@ private boolean processCallbacks(Object bean)
109110
if (serviceAnnotation == null)
110111
return false;
111112

113+
if (!Modifier.isPublic(klass.getModifiers()))
114+
throw new IllegalArgumentException("Service class '" + klass.getName() + "' must be public");
115+
112116
boolean result = processListener(bean);
113117
result |= processSubscription(bean);
114118
return result;
@@ -181,7 +185,7 @@ private boolean processSession(Object bean, ClientSession clientSession)
181185
{
182186
setField(bean, field, clientSession);
183187
result = true;
184-
logger.debug("Injected {} to field {} on bean {}", new Object[]{clientSession, field, bean});
188+
logger.debug("Injected {} to field {} on bean {}", clientSession, field, bean);
185189
}
186190
}
187191
}
@@ -198,7 +202,7 @@ private boolean processSession(Object bean, ClientSession clientSession)
198202
{
199203
invokeMethod(bean, method, clientSession);
200204
result = true;
201-
logger.debug("Injected {} to method {} on bean {}", new Object[]{clientSession, method, bean});
205+
logger.debug("Injected {} to method {} on bean {}", clientSession, method, bean);
202206
}
203207
}
204208
}
@@ -218,6 +222,9 @@ private boolean processListener(Object bean)
218222
Listener listener = method.getAnnotation(Listener.class);
219223
if (listener != null)
220224
{
225+
if (!Modifier.isPublic(method.getModifiers()))
226+
throw new IllegalArgumentException("Service method '" + method.getName() + "' in class '" + method.getDeclaringClass().getName() + "' must be public");
227+
221228
String[] channels = listener.value();
222229
for (String channel : channels)
223230
{
@@ -234,7 +241,7 @@ private boolean processListener(Object bean)
234241
}
235242
callbacks.add(listenerCallback);
236243
result = true;
237-
logger.debug("Registered listener for channel {} to method {} on bean {}", new Object[]{channel, method, bean});
244+
logger.debug("Registered listener for channel {} to method {} on bean {}", channel, method, bean);
238245
}
239246
}
240247
}
@@ -272,6 +279,9 @@ private boolean processSubscription(Object bean)
272279
Subscription subscription = method.getAnnotation(Subscription.class);
273280
if (subscription != null)
274281
{
282+
if (!Modifier.isPublic(method.getModifiers()))
283+
throw new IllegalArgumentException("Service method '" + method.getName() + "' in class '" + method.getDeclaringClass().getName() + "' must be public");
284+
275285
String[] channels = subscription.value();
276286
for (String channel : channels)
277287
{
@@ -292,7 +302,7 @@ private boolean processSubscription(Object bean)
292302
}
293303
callbacks.add(subscriptionCallback);
294304
result = true;
295-
logger.debug("Registered subscriber for channel {} to method {} on bean {}", new Object[]{channel, method, bean});
305+
logger.debug("Registered subscriber for channel {} to method {} on bean {}", channel, method, bean);
296306
}
297307
}
298308
}
@@ -336,24 +346,23 @@ private ListenerCallback(Object target, Method method, String channel)
336346

337347
public void onMessage(ClientSessionChannel channel, Message message)
338348
{
339-
boolean accessible = method.isAccessible();
340349
try
341350
{
342-
method.setAccessible(true);
343351
method.invoke(target, message);
344352
}
345353
catch (InvocationTargetException x)
346354
{
347-
throw new RuntimeException(x.getCause());
355+
Throwable cause = x.getCause();
356+
if (cause instanceof RuntimeException)
357+
throw (RuntimeException)cause;
358+
if (cause instanceof Error)
359+
throw (Error)cause;
360+
throw new RuntimeException(cause);
348361
}
349362
catch (IllegalAccessException x)
350363
{
351364
throw new RuntimeException(x);
352365
}
353-
finally
354-
{
355-
method.setAccessible(accessible);
356-
}
357366
}
358367
}
359368

@@ -399,24 +408,23 @@ private void subscribe()
399408

400409
private void forward(Message message)
401410
{
402-
boolean accessible = method.isAccessible();
403411
try
404412
{
405-
method.setAccessible(true);
406413
method.invoke(target, message);
407414
}
408415
catch (InvocationTargetException x)
409416
{
410-
throw new RuntimeException(x.getCause());
417+
Throwable cause = x.getCause();
418+
if (cause instanceof RuntimeException)
419+
throw (RuntimeException)cause;
420+
if (cause instanceof Error)
421+
throw (Error)cause;
422+
throw new RuntimeException(cause);
411423
}
412424
catch (IllegalAccessException x)
413425
{
414426
throw new RuntimeException(x);
415427
}
416-
finally
417-
{
418-
method.setAccessible(accessible);
419-
}
420428
}
421429
}
422430
}

‎cometd-java/cometd-java-annotations/src/main/java/org/cometd/annotation/ServerAnnotationProcessor.java‎

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.lang.reflect.Field;
2020
import java.lang.reflect.InvocationTargetException;
2121
import java.lang.reflect.Method;
22+
import java.lang.reflect.Modifier;
2223
import java.util.ArrayList;
2324
import java.util.Arrays;
2425
import java.util.List;
@@ -147,7 +148,7 @@ public void configureChannel(ConfigurableServerChannel channel)
147148
boolean flip = false;
148149
try
149150
{
150-
logger.debug("Configure channel {} with method {} on bean {}", new Object[]{channel, method, bean});
151+
logger.debug("Configure channel {} with method {} on bean {}", channel, method, bean);
151152
if (!method.isAccessible())
152153
{
153154
flip = true;
@@ -171,13 +172,13 @@ public void configureChannel(ConfigurableServerChannel channel)
171172

172173
if (initialized)
173174
{
174-
logger.debug("Channel {} already initialized. Not called method {} on bean {}", new Object[]{channel, method, bean});
175+
logger.debug("Channel {} already initialized. Not called method {} on bean {}", channel, method, bean);
175176
}
176177
else
177178
{
178179
if (configure.configureIfExists())
179180
{
180-
logger.debug("Configure channel {} with method {} on bean {}", new Object[]{channel, method, bean});
181+
logger.debug("Configure channel {} with method {} on bean {}", channel, method, bean);
181182
init.configureChannel(bayeuxServer.getChannel(channel));
182183
}
183184
else if (configure.errorIfExists())
@@ -242,6 +243,9 @@ public boolean processCallbacks(Object bean)
242243
if (serviceAnnotation == null)
243244
return false;
244245

246+
if (!Modifier.isPublic(klass.getModifiers()))
247+
throw new IllegalArgumentException("Service class '" + klass.getName() + "' must be public");
248+
245249
LocalSession session = findOrCreateLocalSession(bean, serviceAnnotation.value());
246250
boolean result = processListener(bean, session);
247251
result |= processSubscription(bean, session);
@@ -335,7 +339,7 @@ else if (field.getType().isAssignableFrom(serverSession.getClass()))
335339
{
336340
setField(bean, field, value);
337341
result = true;
338-
logger.debug("Injected {} to field {} on bean {}", new Object[]{value, field, bean});
342+
logger.debug("Injected {} to field {} on bean {}", value, field, bean);
339343
}
340344
}
341345
}
@@ -358,7 +362,7 @@ else if (parameterTypes[0].isAssignableFrom(serverSession.getClass()))
358362
{
359363
invokeMethod(bean, method, value);
360364
result = true;
361-
logger.debug("Injected {} to method {} on bean {}", new Object[]{value, method, bean});
365+
logger.debug("Injected {} to method {} on bean {}", value, method, bean);
362366
}
363367
}
364368
}
@@ -378,6 +382,9 @@ private boolean processListener(Object bean, LocalSession localSession)
378382
Listener listener = method.getAnnotation(Listener.class);
379383
if (listener != null)
380384
{
385+
if (!Modifier.isPublic(method.getModifiers()))
386+
throw new IllegalArgumentException("Service method '" + method.getName() + "' in class '" + method.getDeclaringClass().getName() + "' must be public");
387+
381388
String[] channels = listener.value();
382389
for (String channel : channels)
383390
{
@@ -395,7 +402,7 @@ private boolean processListener(Object bean, LocalSession localSession)
395402
}
396403
callbacks.add(listenerCallback);
397404
result = true;
398-
logger.debug("Registered listener for channel {} to method {} on bean {}", new Object[]{channel, method, bean});
405+
logger.debug("Registered listener for channel {} to method {} on bean {}", channel, method, bean);
399406
}
400407
}
401408
}
@@ -433,6 +440,9 @@ private boolean processSubscription(Object bean, LocalSession localSession)
433440
Subscription subscription = method.getAnnotation(Subscription.class);
434441
if (subscription != null)
435442
{
443+
if (!Modifier.isPublic(method.getModifiers()))
444+
throw new IllegalArgumentException("Service method '" + method.getName() + "' in class '" + method.getDeclaringClass().getName() + "' must be public");
445+
436446
String[] channels = subscription.value();
437447
for (String channel : channels)
438448
{
@@ -449,7 +459,7 @@ private boolean processSubscription(Object bean, LocalSession localSession)
449459
}
450460
callbacks.add(subscriptionCallback);
451461
result = true;
452-
logger.debug("Registered subscriber for channel {} to method {} on bean {}", new Object[]{channel, method, bean});
462+
logger.debug("Registered subscriber for channel {} to method {} on bean {}", channel, method, bean);
453463
}
454464
}
455465
}
@@ -498,25 +508,24 @@ public boolean onMessage(ServerSession from, ServerChannel channel, ServerMessag
498508
if (from == localSession.getServerSession() && !receiveOwnPublishes)
499509
return true;
500510

501-
boolean accessible = method.isAccessible();
502511
try
503512
{
504-
method.setAccessible(true);
505513
Object result = method.invoke(target, from, message);
506514
return !Boolean.FALSE.equals(result);
507515
}
508516
catch (InvocationTargetException x)
509517
{
510-
throw new RuntimeException(x.getCause());
518+
Throwable cause = x.getCause();
519+
if (cause instanceof RuntimeException)
520+
throw (RuntimeException)cause;
521+
if (cause instanceof Error)
522+
throw (Error)cause;
523+
throw new RuntimeException(cause);
511524
}
512525
catch (IllegalAccessException x)
513526
{
514527
throw new RuntimeException(x);
515528
}
516-
finally
517-
{
518-
method.setAccessible(accessible);
519-
}
520529
}
521530
}
522531

@@ -541,24 +550,23 @@ public SubscriptionCallback(LocalSession localSession, Object target, Method met
541550

542551
public void onMessage(ClientSessionChannel channel, Message message)
543552
{
544-
boolean accessible = method.isAccessible();
545553
try
546554
{
547-
method.setAccessible(true);
548555
method.invoke(target, message);
549556
}
550557
catch (InvocationTargetException x)
551558
{
552-
throw new RuntimeException(x.getCause());
559+
Throwable cause = x.getCause();
560+
if (cause instanceof RuntimeException)
561+
throw (RuntimeException)cause;
562+
if (cause instanceof Error)
563+
throw (Error)cause;
564+
throw new RuntimeException(cause);
553565
}
554566
catch (IllegalAccessException x)
555567
{
556568
throw new RuntimeException(x);
557569
}
558-
finally
559-
{
560-
method.setAccessible(accessible);
561-
}
562570
}
563571
}
564572
}

0 commit comments

Comments
 (0)