Skip to content

Commit 54dd703

Browse files
authored
[GEODE-10519] Security : Remove Unsafe Reflection Breaking Java Module System Encapsulation (#7954)
* Replace reflection-based UnsafeThreadLocal with WeakHashMap implementation - Removed reflection access to ThreadLocal/ThreadLocalMap internals - Implemented cross-thread value lookup using synchronized WeakHashMap - Removed requirement for --add-opens=java.base/java.lang=ALL-UNNAMED - WeakHashMap ensures terminated threads can be garbage collected - Maintains same API and functionality for deadlock detection - All existing tests pass without JVM flag changes This eliminates the fragile reflection-based approach that required special JVM flags and was vulnerable to Java module system changes. The new implementation is safer, more maintainable, and works across all Java versions without requiring internal access. * Remove --add-opens=java.base/java.lang from test configuration - Removed unnecessary JVM flag from geode-test.gradle line 185 - Flag no longer needed after UnsafeThreadLocal refactoring - Tests now run with same security constraints as production - All UnsafeThreadLocal and deadlock tests pass without the flag - Validates that refactoring truly eliminated reflection dependency
1 parent 05104a9 commit 54dd703

3 files changed

Lines changed: 52 additions & 57 deletions

File tree

build-tools/scripts/src/main/groovy/geode-test.gradle

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,6 @@ gradle.taskGraph.whenReady({ graph ->
182182
if (project.hasProperty('testJVMVer') && testJVMVer.toInteger() >= 9) {
183183
jvmArgs += [
184184
"--add-opens=java.base/java.io=ALL-UNNAMED",
185-
"--add-opens=java.base/java.lang=ALL-UNNAMED",
186185
"--add-opens=java.base/java.lang.annotation=ALL-UNNAMED",
187186
"--add-opens=java.base/java.lang.module=ALL-UNNAMED",
188187
"--add-opens=java.base/java.lang.ref=ALL-UNNAMED",

geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java

Lines changed: 48 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -14,72 +14,68 @@
1414
*/
1515
package org.apache.geode.distributed.internal.deadlock;
1616

17-
import java.lang.reflect.Field;
18-
import java.lang.reflect.InvocationTargetException;
19-
import java.lang.reflect.Method;
17+
import java.util.Map;
18+
import java.util.WeakHashMap;
2019

2120
/**
22-
* Most of this thread local is safe to use, except for the getValue(Thread) method. That is not
23-
* guaranteed to be correct. But for our deadlock detection tool I think it's good enough, and this
24-
* class provides a very low overhead way for us to record what thread holds a particular resource.
21+
* A ThreadLocal implementation that allows reading values from arbitrary threads, useful for
22+
* deadlock detection. This implementation uses a WeakHashMap to track values per thread without
23+
* requiring reflection or JVM internal access.
2524
*
25+
* <p>
26+
* Unlike standard ThreadLocal, this class maintains an additional mapping that allows querying the
27+
* value for any thread, not just the current thread. This is useful for deadlock detection where
28+
* we need to inspect what resources other threads are holding.
29+
* </p>
2630
*
31+
* <p>
32+
* The implementation uses WeakHashMap with Thread keys to ensure threads can be garbage collected
33+
* when they terminate, preventing memory leaks.
34+
* </p>
2735
*/
2836
public class UnsafeThreadLocal<T> extends ThreadLocal<T> {
2937
/**
30-
* Dangerous method. Uses reflection to extract the thread local for a given thread.
31-
*
32-
* Unlike get(), this method does not set the initial value if none is found
33-
*
38+
* Maps threads to their values. Uses WeakHashMap so terminated threads can be GC'd. Synchronized
39+
* to ensure thread-safe access.
3440
*/
35-
public T get(Thread thread) {
36-
return (T) get(this, thread);
37-
}
41+
private final Map<Thread, T> threadValues =
42+
java.util.Collections.synchronizedMap(new WeakHashMap<>());
3843

39-
private static Object get(ThreadLocal threadLocal, Thread thread) {
40-
try {
41-
Object threadLocalMap =
42-
invokePrivate(threadLocal, "getMap", new Class[] {Thread.class}, new Object[] {thread});
43-
44-
if (threadLocalMap != null) {
45-
Object entry = invokePrivate(threadLocalMap, "getEntry", new Class[] {ThreadLocal.class},
46-
new Object[] {threadLocal});
47-
if (entry != null) {
48-
return getPrivate(entry, "value");
49-
}
50-
}
51-
return null;
52-
} catch (Exception e) {
53-
throw new RuntimeException("Unable to extract thread local", e);
44+
/**
45+
* Sets the value for the current thread and records it in the cross-thread map.
46+
*/
47+
@Override
48+
public void set(T value) {
49+
super.set(value);
50+
if (value != null) {
51+
threadValues.put(Thread.currentThread(), value);
52+
} else {
53+
threadValues.remove(Thread.currentThread());
5454
}
5555
}
5656

57-
private static Object getPrivate(Object object, String fieldName) throws SecurityException,
58-
NoSuchFieldException, IllegalArgumentException, IllegalAccessException {
59-
Field field = object.getClass().getDeclaredField(fieldName);
60-
field.setAccessible(true);
61-
return field.get(object);
57+
/**
58+
* Removes the value for the current thread from both the ThreadLocal and the cross-thread map.
59+
*/
60+
@Override
61+
public void remove() {
62+
super.remove();
63+
threadValues.remove(Thread.currentThread());
6264
}
6365

64-
private static Object invokePrivate(Object object, String methodName, Class[] argTypes,
65-
Object[] args) throws SecurityException, NoSuchMethodException, IllegalArgumentException,
66-
IllegalAccessException, InvocationTargetException {
67-
68-
Method method = null;
69-
Class clazz = object.getClass();
70-
while (method == null) {
71-
try {
72-
method = clazz.getDeclaredMethod(methodName, argTypes);
73-
} catch (NoSuchMethodException e) {
74-
clazz = clazz.getSuperclass();
75-
if (clazz == null) {
76-
throw e;
77-
}
78-
}
79-
}
80-
method.setAccessible(true);
81-
Object result = method.invoke(object, args);
82-
return result;
66+
/**
67+
* Gets the value for an arbitrary thread, useful for deadlock detection.
68+
*
69+
* <p>
70+
* Unlike get(), this method does not set the initial value if none is found. Returns null if the
71+
* specified thread has no value set.
72+
* </p>
73+
*
74+
* @param thread the thread whose value to retrieve
75+
* @return the value for the specified thread, or null if none exists
76+
*/
77+
public T get(Thread thread) {
78+
return threadValues.get(thread);
8379
}
8480

8581
}

geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
import java.util.Collections;
2727
import java.util.List;
2828

29-
import org.apache.geode.distributed.internal.deadlock.UnsafeThreadLocal;
29+
import org.apache.geode.internal.offheap.AddressableMemoryManager;
3030
import org.apache.geode.internal.stats50.VMStats50;
3131
import org.apache.geode.unsafe.internal.com.sun.jmx.remote.security.MBeanServerAccessController;
3232

@@ -38,9 +38,9 @@ public class MemberJvmOptions {
3838
private static final String COM_SUN_JMX_REMOTE_SECURITY_EXPORT =
3939
"--add-exports=java.management/com.sun.jmx.remote.security=ALL-UNNAMED";
4040
/**
41-
* open needed by {@link UnsafeThreadLocal}
41+
* open needed by {@link AddressableMemoryManager}
4242
*/
43-
private static final String JAVA_LANG_OPEN = "--add-opens=java.base/java.lang=ALL-UNNAMED";
43+
private static final String JAVA_NIO_OPEN = "--add-opens=java.base/java.nio=ALL-UNNAMED";
4444
/**
4545
* open needed by {@link VMStats50}
4646
*/
@@ -50,7 +50,7 @@ public class MemberJvmOptions {
5050
static final List<String> JAVA_11_OPTIONS = Arrays.asList(
5151
COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
5252
COM_SUN_MANAGEMENT_INTERNAL_OPEN,
53-
JAVA_LANG_OPEN);
53+
JAVA_NIO_OPEN);
5454

5555
public static List<String> getMemberJvmOptions() {
5656
if (isJavaVersionAtLeast(JAVA_11)) {

0 commit comments

Comments
 (0)