From 95d4f51838ab9f63c904bee6d3aebc1cf6f3b678 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 29 May 2024 11:38:47 +0200 Subject: [PATCH] Ensure locals are in scope when generating metrics Verify that where the metric is inserted, the local variable that we try to access is in scope (from the LocalVariableTable) We are doing this for CapturedContext now we add this for metrics to avoid having a VerifyError raised because the variable is not accessible or reused with another type than expected. --- .../debugger/instrumentation/ASMHelper.java | 19 +++++++ .../CapturedContextInstrumentor.java | 27 ++++------ .../instrumentation/MetricInstrumentor.java | 54 +++++++++++-------- .../MetricProbesInstrumentationTest.java | 24 +++++++++ 4 files changed, 85 insertions(+), 39 deletions(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java index a3f7db28e7f..dff5eef2b6c 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java @@ -16,6 +16,7 @@ import org.objectweb.asm.Type; import org.objectweb.asm.signature.SignatureReader; import org.objectweb.asm.signature.SignatureVisitor; +import org.objectweb.asm.tree.AbstractInsnNode; import org.objectweb.asm.tree.FieldInsnNode; import org.objectweb.asm.tree.FieldNode; import org.objectweb.asm.tree.InsnList; @@ -23,6 +24,7 @@ import org.objectweb.asm.tree.LdcInsnNode; import org.objectweb.asm.tree.LocalVariableNode; import org.objectweb.asm.tree.MethodInsnNode; +import org.objectweb.asm.tree.MethodNode; import org.objectweb.asm.tree.TypeInsnNode; /** Helper class for bytecode generation */ @@ -274,6 +276,23 @@ public static void invokeConstructor( // stack: [] } + /** Checks if the given variable is in scope at the given location */ + public static boolean isInScope( + MethodNode methodNode, LocalVariableNode variableNode, AbstractInsnNode location) { + AbstractInsnNode startScope = + variableNode.start != null ? variableNode.start : methodNode.instructions.getFirst(); + AbstractInsnNode endScope = + variableNode.end != null ? variableNode.end : methodNode.instructions.getLast(); + AbstractInsnNode insn = startScope; + while (insn != null && insn != endScope) { + if (insn == location) { + return true; + } + insn = insn.getNext(); + } + return false; + } + /** Wraps ASM's {@link org.objectweb.asm.Type} with associated generic types */ public static class Type { private final org.objectweb.asm.Type mainType; diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java index 6135fefc1a3..39bec13b85e 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java @@ -191,6 +191,10 @@ private int addExceptionLocal(TryCatchBlockNode catchHandler, MethodNode methodN || current.getType() == AbstractInsnNode.LINE)) { current = current.getNext(); } + if (current == null) { + reportWarning("Cannot add exception local variable to catch block - no instructions."); + return -1; + } if (current.getOpcode() != Opcodes.ASTORE) { reportWarning("Cannot add exception local variable to catch block - no store instruction."); return -1; @@ -673,8 +677,11 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList) List applicableVars = new ArrayList<>(); for (LocalVariableNode variableNode : methodNode.localVariables) { int idx = variableNode.index - localVarBaseOffset; - if (idx >= argOffset && isInScope(variableNode, location)) { - applicableVars.add(variableNode); + if (idx >= argOffset) { + // var is local not arg + if (ASMHelper.isInScope(methodNode, variableNode, location)) { + applicableVars.add(variableNode); + } } } @@ -1072,22 +1079,6 @@ private static void addInheritedStaticFields( } } - private boolean isInScope(LocalVariableNode variableNode, AbstractInsnNode location) { - AbstractInsnNode startScope = - variableNode.start != null ? variableNode.start : methodNode.instructions.getFirst(); - AbstractInsnNode endScope = - variableNode.end != null ? variableNode.end : methodNode.instructions.getLast(); - - AbstractInsnNode insn = startScope; - while (insn != null && insn != endScope) { - if (insn == location) { - return true; - } - insn = insn.getNext(); - } - return false; - } - private int declareContextVar(InsnList insnList) { int var = newVar(CAPTURED_CONTEXT_TYPE); getStatic(insnList, CAPTURED_CONTEXT_TYPE, "EMPTY_CAPTURING_CONTEXT"); diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java index 0417b9a3279..6cbda5302cb 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java @@ -7,6 +7,7 @@ import static com.datadog.debugger.instrumentation.ASMHelper.invokeInterface; import static com.datadog.debugger.instrumentation.ASMHelper.invokeStatic; import static com.datadog.debugger.instrumentation.ASMHelper.invokeVirtual; +import static com.datadog.debugger.instrumentation.ASMHelper.isInScope; import static com.datadog.debugger.instrumentation.ASMHelper.isStaticField; import static com.datadog.debugger.instrumentation.ASMHelper.ldc; import static com.datadog.debugger.instrumentation.Types.*; @@ -101,7 +102,7 @@ public InstrumentationResult.Status instrument() { case ENTRY: case DEFAULT: { - InsnList insnList = wrapTryCatch(callMetric(metricProbe)); + InsnList insnList = wrapTryCatch(callMetric(metricProbe, null)); methodNode.instructions.insert(methodEnterLabel, insnList); break; } @@ -159,7 +160,7 @@ protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) { switch (node.getOpcode()) { case Opcodes.RET: case Opcodes.RETURN: - return wrapTryCatch(callMetric(metricProbe)); + return wrapTryCatch(callMetric(metricProbe, node)); case Opcodes.LRETURN: storeOpCode = Opcodes.LSTORE; loadOpCode = Opcodes.LLOAD; @@ -192,7 +193,8 @@ protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) { } int tmpIdx = newVar(size); InsnList insnList = - wrapTryCatch(callMetric(metricProbe, new ReturnContext(tmpIdx, loadOpCode, returnType))); + wrapTryCatch( + callMetric(metricProbe, node, new ReturnContext(tmpIdx, loadOpCode, returnType))); // store return value from the stack to local before wrapped call insnList.insert(new VarInsnNode(storeOpCode, tmpIdx)); // restore return value to the stack after wrapped call @@ -219,7 +221,8 @@ private void createFinallyHandler(LabelNode startLabel, LabelNode endLabel, Insn finallyBlocks.add(new FinallyBlock(startLabel, endLabel, handlerLabel)); } - private InsnList callCount(MetricProbe metricProbe, ReturnContext returnContext) { + private InsnList callCount( + MetricProbe metricProbe, AbstractInsnNode targetLocation, ReturnContext returnContext) { if (metricProbe.getValue() == null) { InsnList insnList = new InsnList(); ldc(insnList, metricProbe.getProbeId().getEncodedId()); @@ -246,10 +249,11 @@ private InsnList callCount(MetricProbe metricProbe, ReturnContext returnContext) // stack [] return insnList; } - return internalCallMetric(metricProbe, returnContext); + return internalCallMetric(metricProbe, targetLocation, returnContext); } - private InsnList internalCallMetric(MetricProbe metricProbe, ReturnContext returnContext) { + private InsnList internalCallMetric( + MetricProbe metricProbe, AbstractInsnNode targetLocation, ReturnContext returnContext) { InsnList insnList = new InsnList(); InsnList nullBranch = new InsnList(); VisitorResult result; @@ -259,7 +263,7 @@ private InsnList internalCallMetric(MetricProbe metricProbe, ReturnContext retur metricProbe .getValue() .getExpr() - .accept(new MetricValueVisitor(this, nullBranch, returnContext)); + .accept(new MetricValueVisitor(this, nullBranch, targetLocation, returnContext)); } catch (InvalidValueException | UnsupportedOperationException ex) { reportError(ex.getMessage()); return EMPTY_INSN_LIST; @@ -320,21 +324,22 @@ private Type convertIfRequired(Type currentType, InsnList insnList) { } } - private InsnList callMetric(MetricProbe metricProbe) { - return callMetric(metricProbe, null); + private InsnList callMetric(MetricProbe metricProbe, AbstractInsnNode targetLocation) { + return callMetric(metricProbe, targetLocation, null); } - private InsnList callMetric(MetricProbe metricProbe, ReturnContext returnContext) { + private InsnList callMetric( + MetricProbe metricProbe, AbstractInsnNode targetLocation, ReturnContext returnContext) { switch (metricProbe.getKind()) { case COUNT: - return callCount(metricProbe, returnContext); + return callCount(metricProbe, targetLocation, returnContext); case GAUGE: case HISTOGRAM: case DISTRIBUTION: if (metricProbe.getValue() == null) { return EMPTY_INSN_LIST; } - return internalCallMetric(metricProbe, returnContext); + return internalCallMetric(metricProbe, targetLocation, returnContext); default: reportError(String.format("Unknown metric kind: %s", metricProbe.getKind())); } @@ -363,11 +368,11 @@ private InstrumentationResult.Status addLineMetric(ClassFileLines classFileLines "No line info for " + (sourceLine.isSingleLine() ? "line " : "range ") + sourceLine); } if (beforeLabel != null) { - InsnList insnList = wrapTryCatch(callMetric(metricProbe)); + InsnList insnList = wrapTryCatch(callMetric(metricProbe, beforeLabel)); methodNode.instructions.insertBefore(beforeLabel.getNext(), insnList); } if (afterLabel != null && !sourceLine.isSingleLine()) { - InsnList insnList = wrapTryCatch(callMetric(metricProbe)); + InsnList insnList = wrapTryCatch(callMetric(metricProbe, afterLabel)); methodNode.instructions.insert(afterLabel, insnList); } } @@ -387,12 +392,17 @@ public VisitorResult(ASMHelper.Type type, InsnList insnList) { private class MetricValueVisitor implements Visitor { private final MetricInstrumentor instrumentor; private final InsnList nullBranch; + private final AbstractInsnNode targetLocation; private final ReturnContext returnContext; public MetricValueVisitor( - MetricInstrumentor instrumentor, InsnList nullBranch, ReturnContext returnContext) { + MetricInstrumentor instrumentor, + InsnList nullBranch, + AbstractInsnNode targetLocation, + ReturnContext returnContext) { this.instrumentor = instrumentor; this.nullBranch = nullBranch; + this.targetLocation = targetLocation; this.returnContext = returnContext; } @@ -709,12 +719,14 @@ private ASMHelper.Type tryRetrieveArgument(String head, InsnList insnList) { private ASMHelper.Type tryRetrieveLocalVar(String head, InsnList insnList) { for (LocalVariableNode varNode : instrumentor.methodNode.localVariables) { if (varNode.name.equals(head)) { - Type varType = Type.getType(varNode.desc); - VarInsnNode varInsnNode = - new VarInsnNode(varType.getOpcode(Opcodes.ILOAD), varNode.index); - insnList.add(varInsnNode); - // stack [local] - return new ASMHelper.Type(varType); + if (isInScope(instrumentor.methodNode, varNode, targetLocation)) { + Type varType = Type.getType(varNode.desc); + VarInsnNode varInsnNode = + new VarInsnNode(varType.getOpcode(Opcodes.ILOAD), varNode.index); + insnList.add(varInsnNode); + // stack [local] + return new ASMHelper.Type(varType); + } } } return null; diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java index 896d563b6f8..2bdc0bb8f23 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java @@ -7,6 +7,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; @@ -17,6 +18,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static utils.InstrumentationTestHelper.compileAndLoadClass; +import static utils.InstrumentationTestHelper.loadClass; import com.datadog.debugger.el.DSL; import com.datadog.debugger.el.ValueScript; @@ -1194,6 +1196,27 @@ public void primitivesFunction() throws IOException, URISyntaxException { assertEquals(97, listener.counters.get(METRIC_NAME_CHAR)); } + @Test + public void localVarNotInScope() throws IOException, URISyntaxException { + final String METRIC_NAME = "lenstr"; + final String CLASS_NAME = "com.datadog.debugger.jaxrs.MyResource"; + MetricProbe metricProbe = + createMetricBuilder(METRIC_ID, METRIC_NAME, GAUGE) + .where(CLASS_NAME, "createResource", null) + .valueScript(new ValueScript(DSL.len(DSL.ref("varStr")), "len(varStr)")) + .build(); + MetricForwarderListener listener = installMetricProbes(metricProbe); + Class testClass = + loadClass(CLASS_NAME, getClass().getResource("/MyResource.class").getFile()); + Object result = + Reflect.onClass(testClass) + .create() + .call("createResource", (Object) null, (Object) null, 1) + .get(); + assertNotNull(result); + assertFalse(listener.gauges.containsKey(METRIC_NAME)); + } + private MetricForwarderListener installSingleMetric( String metricName, MetricProbe.MetricKind metricKind, @@ -1273,6 +1296,7 @@ private MetricForwarderListener installMetricProbes(Configuration configuration) Config config = mock(Config.class); when(config.isDebuggerEnabled()).thenReturn(true); when(config.isDebuggerClassFileDumpEnabled()).thenReturn(true); + when(config.isDebuggerVerifyByteCode()).thenReturn(true); when(config.getFinalDebuggerSnapshotUrl()) .thenReturn("http://localhost:8126/debugger/v1/input"); when(config.getFinalDebuggerSymDBUrl()).thenReturn("http://localhost:8126/symdb/v1/input");