Skip to content

Commit 62a7002

Browse files
jpbempeljpbempel
committed
Fix inaccessible fields capture
NoSuchFieldException can be thrown if we try to capture, through reflection, (static) fields from JDK classes or from another module. To fix this we need to catch eventual exception thrown by accessing field values. For that we continue to try to access directly to field when possible because it will be faster, but when we detect fields are not accessible we use another method that try to access it through reflection but could catch exception if not and returns a CapturedValue instance filled with the reason of this exception.
1 parent ca680ba commit 62a7002

4 files changed

Lines changed: 226 additions & 44 deletions

File tree

‎dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/el/ReflectiveFieldValueResolver.java‎

Lines changed: 56 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
package datadog.trace.bootstrap.debugger.el;
22

3+
import datadog.trace.bootstrap.debugger.CapturedContext;
34
import java.lang.reflect.Field;
45
import java.lang.reflect.Modifier;
56

67
/** A helper class to resolve a reference path using reflection. */
78
public class ReflectiveFieldValueResolver {
89
public static Object resolve(Object target, Class<?> targetType, String fldName) {
9-
Field fld = getField(targetType, fldName);
10+
Field fld = safeGetField(targetType, fldName);
1011
if (fld == null) {
1112
return Values.UNDEFINED_OBJECT;
1213
}
@@ -22,9 +23,41 @@ public static Object getFieldValue(Object target, String fieldName)
2223
return getField(target, fieldName).get(target);
2324
}
2425

26+
public static CapturedContext.CapturedValue getFieldAsCapturedValue(
27+
Object target, String fieldName) {
28+
if (target == null) {
29+
return CapturedContext.CapturedValue.of(fieldName, Object.class.getTypeName(), null);
30+
}
31+
return getFieldAsCapturedValue(target.getClass(), target, fieldName);
32+
}
33+
34+
public static CapturedContext.CapturedValue getFieldAsCapturedValue(
35+
Class<?> clazz, Object target, String fieldName) {
36+
Field field;
37+
try {
38+
field = getField(clazz, fieldName);
39+
if (field == null) {
40+
return CapturedContext.CapturedValue.notCapturedReason(
41+
fieldName, Object.class.getTypeName(), "Field not found");
42+
}
43+
} catch (Exception ex) {
44+
return CapturedContext.CapturedValue.notCapturedReason(
45+
fieldName, Object.class.getTypeName(), ex.toString());
46+
}
47+
String declaredFieldType = Object.class.getTypeName();
48+
try {
49+
declaredFieldType = field.getType().getTypeName();
50+
Object fieldValue = field.get(target);
51+
return CapturedContext.CapturedValue.of(fieldName, declaredFieldType, fieldValue);
52+
} catch (Exception ex) {
53+
return CapturedContext.CapturedValue.notCapturedReason(
54+
fieldName, declaredFieldType, ex.toString());
55+
}
56+
}
57+
2558
public static Object getFieldValue(Class<?> targetClass, String fieldName)
2659
throws IllegalAccessException {
27-
return getField(targetClass, fieldName).get(null);
60+
return safeGetField(targetClass, fieldName).get(null);
2861
}
2962

3063
public static long getFieldValueAsLong(Object target, String fieldName)
@@ -34,7 +67,7 @@ public static long getFieldValueAsLong(Object target, String fieldName)
3467

3568
public static long getFieldValueAsLong(Class<?> targetClass, String fieldName)
3669
throws IllegalAccessException {
37-
return getField(targetClass, fieldName).getLong(null);
70+
return safeGetField(targetClass, fieldName).getLong(null);
3871
}
3972

4073
public static int getFieldValueAsInt(Object target, String fieldName)
@@ -44,7 +77,7 @@ public static int getFieldValueAsInt(Object target, String fieldName)
4477

4578
public static int getFieldValueAsInt(Class<?> targetClass, String fieldName)
4679
throws IllegalAccessException {
47-
return getField(targetClass, fieldName).getInt(null);
80+
return safeGetField(targetClass, fieldName).getInt(null);
4881
}
4982

5083
public static double getFieldValueAsDouble(Object target, String fieldName)
@@ -54,7 +87,7 @@ public static double getFieldValueAsDouble(Object target, String fieldName)
5487

5588
public static double getFieldValueAsDouble(Class<?> targetClass, String fieldName)
5689
throws IllegalAccessException {
57-
return getField(targetClass, fieldName).getDouble(null);
90+
return safeGetField(targetClass, fieldName).getDouble(null);
5891
}
5992

6093
public static float getFieldValueAsFloat(Object target, String fieldName)
@@ -64,7 +97,7 @@ public static float getFieldValueAsFloat(Object target, String fieldName)
6497

6598
public static float getFieldValueAsFloat(Class<?> targetClass, String fieldName)
6699
throws IllegalAccessException {
67-
return getField(targetClass, fieldName).getFloat(null);
100+
return safeGetField(targetClass, fieldName).getFloat(null);
68101
}
69102

70103
public static float getFieldValueAsShort(Object target, String fieldName)
@@ -74,7 +107,7 @@ public static float getFieldValueAsShort(Object target, String fieldName)
74107

75108
public static float getFieldValueAsShort(Class<?> targetClass, String fieldName)
76109
throws IllegalAccessException {
77-
return getField(targetClass, fieldName).getShort(null);
110+
return safeGetField(targetClass, fieldName).getShort(null);
78111
}
79112

80113
public static char getFieldValueAsChar(Object target, String fieldName)
@@ -84,7 +117,7 @@ public static char getFieldValueAsChar(Object target, String fieldName)
84117

85118
public static char getFieldValueAsChar(Class<?> targetClass, String fieldName)
86119
throws IllegalAccessException {
87-
return getField(targetClass, fieldName).getChar(null);
120+
return safeGetField(targetClass, fieldName).getChar(null);
88121
}
89122

90123
public static byte getFieldValueAsByte(Object target, String fieldName)
@@ -94,7 +127,7 @@ public static byte getFieldValueAsByte(Object target, String fieldName)
94127

95128
public static byte getFieldValueAsByte(Class<?> targetClass, String fieldName)
96129
throws IllegalAccessException {
97-
return getField(targetClass, fieldName).getByte(null);
130+
return safeGetField(targetClass, fieldName).getByte(null);
98131
}
99132

100133
public static boolean getFieldValueAsBoolean(Object target, String fieldName)
@@ -104,20 +137,32 @@ public static boolean getFieldValueAsBoolean(Object target, String fieldName)
104137

105138
public static boolean getFieldValueAsBoolean(Class<?> targetClass, String fieldName)
106139
throws IllegalAccessException {
107-
return getField(targetClass, fieldName).getBoolean(null);
140+
return safeGetField(targetClass, fieldName).getBoolean(null);
108141
}
109142

110143
private static Field getField(Object target, String name) throws NoSuchFieldException {
111144
if (target == null) {
112145
throw new NullPointerException();
113146
}
114-
Field field = getField(target.getClass(), name);
147+
Field field = safeGetField(target.getClass(), name);
115148
if (field == null) {
116149
throw new NoSuchFieldException(name);
117150
}
118151
return field;
119152
}
120153

154+
private static Field safeGetField(Class<?> container, String name) {
155+
try {
156+
return getField(container, name);
157+
} catch (SecurityException ignored) {
158+
return null;
159+
} catch (Exception e) {
160+
// The only other exception allowed here is InaccessibleObjectException but since we compile
161+
// against JDK 8 we can not use that type in the exception handler
162+
return null;
163+
}
164+
}
165+
121166
private static Field getField(Class<?> container, String name) {
122167
while (container != null) {
123168
try {
@@ -126,12 +171,6 @@ private static Field getField(Class<?> container, String name) {
126171
return fld;
127172
} catch (NoSuchFieldException ignored) {
128173
container = container.getSuperclass();
129-
} catch (SecurityException ignored) {
130-
return null;
131-
} catch (Exception ignored) {
132-
// The only other exception allowed here is InaccessibleObjectException but since we compile
133-
// against JDK 8 we can not use that type in the exception handler
134-
return null;
135174
}
136175
}
137176
return null;

‎dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java‎

Lines changed: 49 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package com.datadog.debugger.instrumentation;
22

3-
import static com.datadog.debugger.instrumentation.ASMHelper.emitReflectiveCall;
43
import static com.datadog.debugger.instrumentation.ASMHelper.getStatic;
54
import static com.datadog.debugger.instrumentation.ASMHelper.invokeConstructor;
65
import static com.datadog.debugger.instrumentation.ASMHelper.invokeStatic;
@@ -17,6 +16,7 @@
1716
import static com.datadog.debugger.instrumentation.Types.DEBUGGER_CONTEXT_TYPE;
1817
import static com.datadog.debugger.instrumentation.Types.METHOD_LOCATION_TYPE;
1918
import static com.datadog.debugger.instrumentation.Types.OBJECT_TYPE;
19+
import static com.datadog.debugger.instrumentation.Types.REFLECTIVE_FIELD_VALUE_RESOLVER_TYPE;
2020
import static com.datadog.debugger.instrumentation.Types.STRING_ARRAY_TYPE;
2121
import static com.datadog.debugger.instrumentation.Types.STRING_TYPE;
2222
import static com.datadog.debugger.instrumentation.Types.THROWABLE_TYPE;
@@ -793,20 +793,30 @@ private void collectStaticFields(InsnList insnList) {
793793
// stack: [capturedcontext, capturedcontext, array, array]
794794
ldc(insnList, counter++);
795795
// stack: [capturedcontext, capturedcontext, array, array, int]
796+
if (!isAccessible(fieldNode)) {
797+
ldc(insnList, Type.getObjectType(classNode.name));
798+
ldc(insnList, null);
799+
ldc(insnList, fieldNode.name);
800+
// stack: [capturedcontext, capturedcontext, array, array, int, null, string]
801+
invokeStatic(
802+
insnList,
803+
REFLECTIVE_FIELD_VALUE_RESOLVER_TYPE,
804+
"getFieldAsCapturedValue",
805+
CAPTURED_VALUE,
806+
CLASS_TYPE,
807+
OBJECT_TYPE,
808+
STRING_TYPE);
809+
insnList.add(new InsnNode(Opcodes.AASTORE));
810+
// stack: [capturedcontext, capturedcontext, array]
811+
continue;
812+
}
796813
ldc(insnList, fieldNode.name);
797814
// stack: [capturedcontext, capturedcontext, array, array, int, string]
798815
Type fieldType = Type.getType(fieldNode.desc);
799816
ldc(insnList, fieldType.getClassName());
800817
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name]
801-
if (isAccessible(fieldNode)) {
802-
insnList.add(
803-
new FieldInsnNode(Opcodes.GETSTATIC, classNode.name, fieldNode.name, fieldNode.desc));
804-
} else {
805-
ldc(insnList, Type.getObjectType(classNode.name));
806-
ldc(insnList, fieldNode.name);
807-
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name, string]
808-
emitReflectiveCall(insnList, new ASMHelper.Type(Type.getType(fieldNode.desc)), CLASS_TYPE);
809-
}
818+
insnList.add(
819+
new FieldInsnNode(Opcodes.GETSTATIC, classNode.name, fieldNode.name, fieldNode.desc));
810820
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
811821
// field_value]
812822
tryBox(fieldType, insnList);
@@ -855,6 +865,23 @@ private void collectFields(InsnList insnList) {
855865
// stack: [capturedcontext, capturedcontext, array, array]
856866
ldc(insnList, counter++);
857867
// stack: [capturedcontext, capturedcontext, array, array, int]
868+
if (!isAccessible(fieldNode)) {
869+
insnList.add(new VarInsnNode(Opcodes.ALOAD, 0));
870+
// stack: [capturedcontext, capturedcontext, array, array, int, this]
871+
ldc(insnList, fieldNode.name);
872+
// stack: [capturedcontext, capturedcontext, array, array, int, this, string]
873+
invokeStatic(
874+
insnList,
875+
REFLECTIVE_FIELD_VALUE_RESOLVER_TYPE,
876+
"getFieldAsCapturedValue",
877+
CAPTURED_VALUE,
878+
OBJECT_TYPE,
879+
STRING_TYPE);
880+
// stack: [capturedcontext, capturedcontext, array, array, int, CapturedValue]
881+
insnList.add(new InsnNode(Opcodes.AASTORE));
882+
// stack: [capturedcontext, capturedcontext, array]
883+
continue;
884+
}
858885
ldc(insnList, fieldNode.name);
859886
// stack: [capturedcontext, capturedcontext, array, array, int, string]
860887
Type fieldType = Type.getType(fieldNode.desc);
@@ -867,6 +894,8 @@ private void collectFields(InsnList insnList) {
867894
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
868895
// access]
869896
invokeVirtual(insnList, CORRELATION_ACCESS_TYPE, "getTraceId", STRING_TYPE);
897+
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
898+
// field_value]
870899
break;
871900
}
872901
case "dd.span_id":
@@ -875,33 +904,26 @@ private void collectFields(InsnList insnList) {
875904
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
876905
// access]
877906
invokeVirtual(insnList, CORRELATION_ACCESS_TYPE, "getSpanId", STRING_TYPE);
907+
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
908+
// field_value]
878909
break;
879910
}
880911
default:
881912
{
882913
insnList.add(new VarInsnNode(Opcodes.ALOAD, 0));
883-
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name, this]
884-
if (isAccessible(fieldNode)) {
885-
insnList.add(
886-
new FieldInsnNode(
887-
Opcodes.GETFIELD, classNode.name, fieldNode.name, fieldNode.desc));
888-
} else {
889-
ldc(insnList, fieldNode.name);
890-
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
891-
// this, string]
892-
emitReflectiveCall(
893-
insnList, new ASMHelper.Type(Type.getType(fieldNode.desc)), OBJECT_TYPE);
894-
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
895-
// this, string]
896-
}
914+
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
915+
// this]
916+
insnList.add(
917+
new FieldInsnNode(
918+
Opcodes.GETFIELD, classNode.name, fieldNode.name, fieldNode.desc));
919+
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
920+
// field_value]
897921
}
898922
}
899-
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
900-
// field_value]
901923
tryBox(fieldType, insnList);
902924
// stack: [capturedcontext, capturedcontext, array, array, int, type_name, object]
903925
addCapturedValueOf(insnList, limits);
904-
// stack: [capturedcontext, capturedcontext, array, array, int, typed_value]
926+
// stack: [capturedcontext, capturedcontext, array, array, int, CapturedValue]
905927
insnList.add(new InsnNode(Opcodes.AASTORE));
906928
// stack: [capturedcontext, capturedcontext, array]
907929
}

‎dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@
9090
import org.junit.jupiter.api.BeforeEach;
9191
import org.junit.jupiter.api.Disabled;
9292
import org.junit.jupiter.api.Test;
93+
import org.junit.jupiter.api.condition.EnabledForJreRange;
9394
import org.junit.jupiter.api.condition.EnabledOnJre;
9495
import org.junit.jupiter.api.condition.JRE;
9596
import org.junit.jupiter.params.ParameterizedTest;
@@ -759,6 +760,46 @@ public void fieldExtractorCount2() throws IOException, URISyntaxException {
759760
assertTrue(compositeDataFields.containsKey("s2"));
760761
}
761762

763+
@Test
764+
@EnabledForJreRange(min = JRE.JAVA_17)
765+
public void fieldExtractorNotAccessible() throws IOException, URISyntaxException {
766+
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot30";
767+
LogProbe logProbe = createProbe(PROBE_ID, CLASS_NAME + "$MyObjectInputStream", "process", "()");
768+
TestSnapshotListener listener = installProbes(CLASS_NAME, logProbe);
769+
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
770+
int result = Reflect.onClass(testClass).call("main", "").get();
771+
assertEquals(42, result);
772+
Snapshot snapshot = assertOneSnapshot(listener);
773+
assertCaptureFieldsNotCaptured(
774+
snapshot.getCaptures().getReturn(),
775+
"bin",
776+
"java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.io.ObjectInputStream\\$BlockDataInputStream java.io.ObjectInputStream.bin accessible: module java.base does not \"opens java.io\" to unnamed module.*");
777+
assertCaptureFieldsNotCaptured(
778+
snapshot.getCaptures().getReturn(),
779+
"vlist",
780+
"java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.io.ObjectInputStream\\$ValidationList java.io.ObjectInputStream.vlist accessible: module java.base does not \"opens java.io\" to unnamed module.*");
781+
}
782+
783+
@Test
784+
@EnabledForJreRange(min = JRE.JAVA_17)
785+
public void staticFieldExtractorNotAccessible() throws IOException, URISyntaxException {
786+
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot30";
787+
LogProbe logProbe = createProbe(PROBE_ID, CLASS_NAME + "$MyHttpURLConnection", "process", "()");
788+
TestSnapshotListener listener = installProbes(CLASS_NAME, logProbe);
789+
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
790+
int result = Reflect.onClass(testClass).call("main", "static").get();
791+
assertEquals(42, result);
792+
Snapshot snapshot = assertOneSnapshot(listener);
793+
assertCaptureStaticFieldsNotCaptured(
794+
snapshot.getCaptures().getReturn(),
795+
"followRedirects",
796+
"java.lang.reflect.InaccessibleObjectException: Unable to make field private static boolean java.net.HttpURLConnection.followRedirects accessible: module java.base does not \"opens java.net\" to unnamed module.*");
797+
assertCaptureStaticFieldsNotCaptured(
798+
snapshot.getCaptures().getReturn(),
799+
"factory",
800+
"java.lang.reflect.InaccessibleObjectException: Unable to make field private static volatile java.net.ContentHandlerFactory java.net.URLConnection.factory accessible: module java.base does not \"opens java.net\" to unnamed module.*");
801+
}
802+
762803
@Test
763804
public void uncaughtException() throws IOException, URISyntaxException {
764805
final String CLASS_NAME = "CapturedSnapshot05";
@@ -2323,6 +2364,20 @@ private void assertCaptureFields(
23232364
}
23242365
}
23252366

2367+
private void assertCaptureFieldsNotCaptured(
2368+
CapturedContext context, String name, String expectedReasonRegEx) {
2369+
CapturedContext.CapturedValue field = context.getFields().get(name);
2370+
assertTrue(
2371+
field.getNotCapturedReason().matches(expectedReasonRegEx), field.getNotCapturedReason());
2372+
}
2373+
2374+
private void assertCaptureStaticFieldsNotCaptured(
2375+
CapturedContext context, String name, String expectedReasonRegEx) {
2376+
CapturedContext.CapturedValue field = context.getStaticFields().get(name);
2377+
assertTrue(
2378+
field.getNotCapturedReason().matches(expectedReasonRegEx), field.getNotCapturedReason());
2379+
}
2380+
23262381
private void assertCaptureFieldCount(CapturedContext context, int expectedFieldCount) {
23272382
assertEquals(expectedFieldCount, context.getFields().size());
23282383
}

0 commit comments

Comments
 (0)