Skip to content

Commit

Permalink
Merge pull request #6883 from DataDog/jpbempel/fix-field-collection
Browse files Browse the repository at this point in the history 
Fix inaccessible fields capture
  • Loading branch information
@jpbempel
jpbempel authored Apr 4, 2024
2 parents 396fcca + 62a7002 commit c1ae1bc
Show file tree
Hide file tree
Showing 4 changed files with 226 additions and 44 deletions.
73 changes: 56 additions & 17 deletions ...strap/src/main/java/datadog/trace/bootstrap/debugger/el/ReflectiveFieldValueResolver.java
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package datadog.trace.bootstrap.debugger.el;

import datadog.trace.bootstrap.debugger.CapturedContext;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;

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

public static CapturedContext.CapturedValue getFieldAsCapturedValue(
Object target, String fieldName) {
if (target == null) {
return CapturedContext.CapturedValue.of(fieldName, Object.class.getTypeName(), null);
}
return getFieldAsCapturedValue(target.getClass(), target, fieldName);
}

public static CapturedContext.CapturedValue getFieldAsCapturedValue(
Class<?> clazz, Object target, String fieldName) {
Field field;
try {
field = getField(clazz, fieldName);
if (field == null) {
return CapturedContext.CapturedValue.notCapturedReason(
fieldName, Object.class.getTypeName(), "Field not found");
}
} catch (Exception ex) {
return CapturedContext.CapturedValue.notCapturedReason(
fieldName, Object.class.getTypeName(), ex.toString());
}
String declaredFieldType = Object.class.getTypeName();
try {
declaredFieldType = field.getType().getTypeName();
Object fieldValue = field.get(target);
return CapturedContext.CapturedValue.of(fieldName, declaredFieldType, fieldValue);
} catch (Exception ex) {
return CapturedContext.CapturedValue.notCapturedReason(
fieldName, declaredFieldType, ex.toString());
}
}

public static Object getFieldValue(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).get(null);
return safeGetField(targetClass, fieldName).get(null);
}

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

public static long getFieldValueAsLong(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getLong(null);
return safeGetField(targetClass, fieldName).getLong(null);
}

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

public static int getFieldValueAsInt(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getInt(null);
return safeGetField(targetClass, fieldName).getInt(null);
}

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

public static double getFieldValueAsDouble(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getDouble(null);
return safeGetField(targetClass, fieldName).getDouble(null);
}

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

public static float getFieldValueAsFloat(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getFloat(null);
return safeGetField(targetClass, fieldName).getFloat(null);
}

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

public static float getFieldValueAsShort(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getShort(null);
return safeGetField(targetClass, fieldName).getShort(null);
}

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

public static char getFieldValueAsChar(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getChar(null);
return safeGetField(targetClass, fieldName).getChar(null);
}

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

public static byte getFieldValueAsByte(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getByte(null);
return safeGetField(targetClass, fieldName).getByte(null);
}

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

public static boolean getFieldValueAsBoolean(Class<?> targetClass, String fieldName)
throws IllegalAccessException {
return getField(targetClass, fieldName).getBoolean(null);
return safeGetField(targetClass, fieldName).getBoolean(null);
}

private static Field getField(Object target, String name) throws NoSuchFieldException {
if (target == null) {
throw new NullPointerException();
}
Field field = getField(target.getClass(), name);
Field field = safeGetField(target.getClass(), name);
if (field == null) {
throw new NoSuchFieldException(name);
}
return field;
}

private static Field safeGetField(Class<?> container, String name) {
try {
return getField(container, name);
} catch (SecurityException ignored) {
return null;
} catch (Exception e) {
// The only other exception allowed here is InaccessibleObjectException but since we compile
// against JDK 8 we can not use that type in the exception handler
return null;
}
}

private static Field getField(Class<?> container, String name) {
while (container != null) {
try {
Expand All @@ -126,12 +171,6 @@ private static Field getField(Class<?> container, String name) {
return fld;
} catch (NoSuchFieldException ignored) {
container = container.getSuperclass();
} catch (SecurityException ignored) {
return null;
} catch (Exception ignored) {
// The only other exception allowed here is InaccessibleObjectException but since we compile
// against JDK 8 we can not use that type in the exception handler
return null;
}
}
return null;
Expand Down
76 changes: 49 additions & 27 deletions ...ugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.datadog.debugger.instrumentation;

import static com.datadog.debugger.instrumentation.ASMHelper.emitReflectiveCall;
import static com.datadog.debugger.instrumentation.ASMHelper.getStatic;
import static com.datadog.debugger.instrumentation.ASMHelper.invokeConstructor;
import static com.datadog.debugger.instrumentation.ASMHelper.invokeStatic;
Expand All @@ -17,6 +16,7 @@
import static com.datadog.debugger.instrumentation.Types.DEBUGGER_CONTEXT_TYPE;
import static com.datadog.debugger.instrumentation.Types.METHOD_LOCATION_TYPE;
import static com.datadog.debugger.instrumentation.Types.OBJECT_TYPE;
import static com.datadog.debugger.instrumentation.Types.REFLECTIVE_FIELD_VALUE_RESOLVER_TYPE;
import static com.datadog.debugger.instrumentation.Types.STRING_ARRAY_TYPE;
import static com.datadog.debugger.instrumentation.Types.STRING_TYPE;
import static com.datadog.debugger.instrumentation.Types.THROWABLE_TYPE;
Expand Down Expand Up @@ -793,20 +793,30 @@ private void collectStaticFields(InsnList insnList) {
// stack: [capturedcontext, capturedcontext, array, array]
ldc(insnList, counter++);
// stack: [capturedcontext, capturedcontext, array, array, int]
if (!isAccessible(fieldNode)) {
ldc(insnList, Type.getObjectType(classNode.name));
ldc(insnList, null);
ldc(insnList, fieldNode.name);
// stack: [capturedcontext, capturedcontext, array, array, int, null, string]
invokeStatic(
insnList,
REFLECTIVE_FIELD_VALUE_RESOLVER_TYPE,
"getFieldAsCapturedValue",
CAPTURED_VALUE,
CLASS_TYPE,
OBJECT_TYPE,
STRING_TYPE);
insnList.add(new InsnNode(Opcodes.AASTORE));
// stack: [capturedcontext, capturedcontext, array]
continue;
}
ldc(insnList, fieldNode.name);
// stack: [capturedcontext, capturedcontext, array, array, int, string]
Type fieldType = Type.getType(fieldNode.desc);
ldc(insnList, fieldType.getClassName());
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name]
if (isAccessible(fieldNode)) {
insnList.add(
new FieldInsnNode(Opcodes.GETSTATIC, classNode.name, fieldNode.name, fieldNode.desc));
} else {
ldc(insnList, Type.getObjectType(classNode.name));
ldc(insnList, fieldNode.name);
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name, string]
emitReflectiveCall(insnList, new ASMHelper.Type(Type.getType(fieldNode.desc)), CLASS_TYPE);
}
insnList.add(
new FieldInsnNode(Opcodes.GETSTATIC, classNode.name, fieldNode.name, fieldNode.desc));
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// field_value]
tryBox(fieldType, insnList);
Expand Down Expand Up @@ -855,6 +865,23 @@ private void collectFields(InsnList insnList) {
// stack: [capturedcontext, capturedcontext, array, array]
ldc(insnList, counter++);
// stack: [capturedcontext, capturedcontext, array, array, int]
if (!isAccessible(fieldNode)) {
insnList.add(new VarInsnNode(Opcodes.ALOAD, 0));
// stack: [capturedcontext, capturedcontext, array, array, int, this]
ldc(insnList, fieldNode.name);
// stack: [capturedcontext, capturedcontext, array, array, int, this, string]
invokeStatic(
insnList,
REFLECTIVE_FIELD_VALUE_RESOLVER_TYPE,
"getFieldAsCapturedValue",
CAPTURED_VALUE,
OBJECT_TYPE,
STRING_TYPE);
// stack: [capturedcontext, capturedcontext, array, array, int, CapturedValue]
insnList.add(new InsnNode(Opcodes.AASTORE));
// stack: [capturedcontext, capturedcontext, array]
continue;
}
ldc(insnList, fieldNode.name);
// stack: [capturedcontext, capturedcontext, array, array, int, string]
Type fieldType = Type.getType(fieldNode.desc);
Expand All @@ -867,6 +894,8 @@ private void collectFields(InsnList insnList) {
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// access]
invokeVirtual(insnList, CORRELATION_ACCESS_TYPE, "getTraceId", STRING_TYPE);
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// field_value]
break;
}
case "dd.span_id":
Expand All @@ -875,33 +904,26 @@ private void collectFields(InsnList insnList) {
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// access]
invokeVirtual(insnList, CORRELATION_ACCESS_TYPE, "getSpanId", STRING_TYPE);
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// field_value]
break;
}
default:
{
insnList.add(new VarInsnNode(Opcodes.ALOAD, 0));
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name, this]
if (isAccessible(fieldNode)) {
insnList.add(
new FieldInsnNode(
Opcodes.GETFIELD, classNode.name, fieldNode.name, fieldNode.desc));
} else {
ldc(insnList, fieldNode.name);
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// this, string]
emitReflectiveCall(
insnList, new ASMHelper.Type(Type.getType(fieldNode.desc)), OBJECT_TYPE);
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// this, string]
}
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// this]
insnList.add(
new FieldInsnNode(
Opcodes.GETFIELD, classNode.name, fieldNode.name, fieldNode.desc));
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// field_value]
}
}
// stack: [capturedcontext, capturedcontext, array, array, int, string, type_name,
// field_value]
tryBox(fieldType, insnList);
// stack: [capturedcontext, capturedcontext, array, array, int, type_name, object]
addCapturedValueOf(insnList, limits);
// stack: [capturedcontext, capturedcontext, array, array, int, typed_value]
// stack: [capturedcontext, capturedcontext, array, array, int, CapturedValue]
insnList.add(new InsnNode(Opcodes.AASTORE));
// stack: [capturedcontext, capturedcontext, array]
}
Expand Down
55 changes: 55 additions & 0 deletions ...a-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.EnabledForJreRange;
import org.junit.jupiter.api.condition.EnabledOnJre;
import org.junit.jupiter.api.condition.JRE;
import org.junit.jupiter.params.ParameterizedTest;
Expand Down Expand Up @@ -759,6 +760,46 @@ public void fieldExtractorCount2() throws IOException, URISyntaxException {
assertTrue(compositeDataFields.containsKey("s2"));
}

@Test
@EnabledForJreRange(min = JRE.JAVA_17)
public void fieldExtractorNotAccessible() throws IOException, URISyntaxException {
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot30";
LogProbe logProbe = createProbe(PROBE_ID, CLASS_NAME + "$MyObjectInputStream", "process", "()");
TestSnapshotListener listener = installProbes(CLASS_NAME, logProbe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.onClass(testClass).call("main", "").get();
assertEquals(42, result);
Snapshot snapshot = assertOneSnapshot(listener);
assertCaptureFieldsNotCaptured(
snapshot.getCaptures().getReturn(),
"bin",
"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.*");
assertCaptureFieldsNotCaptured(
snapshot.getCaptures().getReturn(),
"vlist",
"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.*");
}

@Test
@EnabledForJreRange(min = JRE.JAVA_17)
public void staticFieldExtractorNotAccessible() throws IOException, URISyntaxException {
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot30";
LogProbe logProbe = createProbe(PROBE_ID, CLASS_NAME + "$MyHttpURLConnection", "process", "()");
TestSnapshotListener listener = installProbes(CLASS_NAME, logProbe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.onClass(testClass).call("main", "static").get();
assertEquals(42, result);
Snapshot snapshot = assertOneSnapshot(listener);
assertCaptureStaticFieldsNotCaptured(
snapshot.getCaptures().getReturn(),
"followRedirects",
"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.*");
assertCaptureStaticFieldsNotCaptured(
snapshot.getCaptures().getReturn(),
"factory",
"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.*");
}

@Test
public void uncaughtException() throws IOException, URISyntaxException {
final String CLASS_NAME = "CapturedSnapshot05";
Expand Down Expand Up @@ -2323,6 +2364,20 @@ private void assertCaptureFields(
}
}

private void assertCaptureFieldsNotCaptured(
CapturedContext context, String name, String expectedReasonRegEx) {
CapturedContext.CapturedValue field = context.getFields().get(name);
assertTrue(
field.getNotCapturedReason().matches(expectedReasonRegEx), field.getNotCapturedReason());
}

private void assertCaptureStaticFieldsNotCaptured(
CapturedContext context, String name, String expectedReasonRegEx) {
CapturedContext.CapturedValue field = context.getStaticFields().get(name);
assertTrue(
field.getNotCapturedReason().matches(expectedReasonRegEx), field.getNotCapturedReason());
}

private void assertCaptureFieldCount(CapturedContext context, int expectedFieldCount) {
assertEquals(expectedFieldCount, context.getFields().size());
}
Expand Down
Loading

0 comments on commit c1ae1bc

Please sign in to comment.