Skip to content

Commit c1ae1bc

Browse files
jpbempeljpbempel
authored
Merge pull request #6883 from DataDog/jpbempel/fix-field-collection
Fix inaccessible fields capture
2 parents 396fcca + 62a7002 commit c1ae1bc

File tree

4 files changed

+226
-44
lines changed

4 files changed

+226
-44
lines changed

‎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)