From 86fd474543a6576d6cba3120d532f66c6067f805 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Fri, 20 Dec 2024 14:16:22 +0100 Subject: [PATCH] Fix var hoisting issue when no previous store In some cases the range of a local var may not have previous init in the immediate neighborhood because of basic block arrangement. Limit the rewirite of previous instruction for store only and looking back only 10 instructions --- .../debugger/instrumentation/ASMHelper.java | 4 ++++ .../CapturedContextInstrumentor.java | 19 ++++++++++++----- .../debugger/agent/CapturedSnapshotTest.java | 20 ++++++++++++------ .../ByteSourceJsonBootstrapper.classfile | Bin 0 -> 10750 bytes 4 files changed, 31 insertions(+), 12 deletions(-) create mode 100644 dd-java-agent/agent-debugger/src/test/resources/classfiles/ByteSourceJsonBootstrapper.classfile 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 5deabf02d8c..885e0c4dd31 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 @@ -340,6 +340,10 @@ public static String extractSuperClass(ClassNode classNode) { return Strings.getClassName(classNode.superName); } + public static boolean isStore(int opcode) { + return opcode >= Opcodes.ISTORE && opcode <= Opcodes.ASTORE; + } + /** 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 48808cd0b2b..f63ad883564 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 @@ -7,6 +7,7 @@ import static com.datadog.debugger.instrumentation.ASMHelper.invokeVirtual; import static com.datadog.debugger.instrumentation.ASMHelper.isFinalField; import static com.datadog.debugger.instrumentation.ASMHelper.isStaticField; +import static com.datadog.debugger.instrumentation.ASMHelper.isStore; import static com.datadog.debugger.instrumentation.ASMHelper.ldc; import static com.datadog.debugger.instrumentation.ASMHelper.newInstance; import static com.datadog.debugger.instrumentation.Types.CAPTURED_CONTEXT_TYPE; @@ -68,7 +69,7 @@ import org.slf4j.LoggerFactory; public class CapturedContextInstrumentor extends Instrumentor { - private static final Logger log = LoggerFactory.getLogger(CapturedContextInstrumentor.class); + private static final Logger LOGGER = LoggerFactory.getLogger(CapturedContextInstrumentor.class); private final boolean captureSnapshot; private final Limits limits; private final LabelNode contextInitLabel = new LabelNode(); @@ -656,11 +657,13 @@ private void rewriteLocalVarInsn(LocalVariableNode localVar, int oldSlot, int ne private static void rewritePreviousStoreInsn( LocalVariableNode localVar, int oldSlot, int newSlot) { AbstractInsnNode previous = localVar.start.getPrevious(); - while (previous != null - && (!(previous instanceof VarInsnNode) || ((VarInsnNode) previous).var != oldSlot)) { + int processed = 0; + // arbitrary fixing limit to 10 previous instructions to look at + while (!isVarStoreForSlot(previous, oldSlot) && processed < 10) { previous = previous.getPrevious(); + processed++; } - if (previous != null) { + if (isVarStoreForSlot(previous, oldSlot)) { VarInsnNode varInsnNode = (VarInsnNode) previous; if (varInsnNode.var == oldSlot) { varInsnNode.var = newSlot; @@ -668,6 +671,12 @@ private static void rewritePreviousStoreInsn( } } + private static boolean isVarStoreForSlot(AbstractInsnNode node, int slotIdx) { + return node instanceof VarInsnNode + && isStore(node.getOpcode()) + && ((VarInsnNode) node).var == slotIdx; + } + private void createInProbeFinallyHandler(LabelNode inProbeStartLabel, LabelNode inProbeEndLabel) { LabelNode handlerLabel = new LabelNode(); InsnList handler = new InsnList(); @@ -1234,7 +1243,7 @@ private static void addInheritedStaticFields( FieldNode fieldNode = new FieldNode(field.getModifiers(), field.getName(), desc, null, field); results.add(fieldNode); - log.debug("Adding static inherited field {}", fieldNode.name); + LOGGER.debug("Adding static inherited field {}", fieldNode.name); fieldCount++; if (fieldCount > limits.maxFieldCount) { return; diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java index 5b82a207785..006dce057fa 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java @@ -66,7 +66,6 @@ import java.lang.reflect.Modifier; import java.net.URISyntaxException; import java.net.URL; -import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -134,6 +133,17 @@ public void methodProbe() throws IOException, URISyntaxException { assertEquals("CapturedSnapshot01.main", snapshot.getStack().get(0).getFunction()); } + @Test + public void localVarHoistingNoPreviousStore() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.fasterxml.jackson.core.json.ByteSourceJsonBootstrapper"; + TestSnapshotListener listener = installSingleProbe(CLASS_NAME, "detectEncoding", null); + Class testClass = + loadClass( + CLASS_NAME, + getClass().getResource("/classfiles/ByteSourceJsonBootstrapper.classfile").getFile()); + assertNotNull(testClass); + } + @Test public void singleLineProbe() throws IOException, URISyntaxException { final String CLASS_NAME = "CapturedSnapshot01"; @@ -219,12 +229,8 @@ public void oldClass1_1() throws Exception { when(config.isDebuggerVerifyByteCode()).thenReturn(true); Class testClass = loadClass( - CLASS_NAME, - Paths.get( - CapturedSnapshotTest.class - .getResource("/classfiles/BooleanUtils.classfile") - .toURI()) - .toString()); + CLASS_NAME, getClass().getResource("/classfiles/BooleanUtils.classfile").getFile()); + boolean result = Reflect.onClass(testClass).call("toBoolean", Boolean.TRUE).get(); assertTrue(result); assertOneSnapshot(listener); diff --git a/dd-java-agent/agent-debugger/src/test/resources/classfiles/ByteSourceJsonBootstrapper.classfile b/dd-java-agent/agent-debugger/src/test/resources/classfiles/ByteSourceJsonBootstrapper.classfile new file mode 100644 index 0000000000000000000000000000000000000000..e976133617332cfda9b8d99eadfe7f301713eaa0 GIT binary patch literal 10750 zcmb_i33yZ2vHs^uwq#khWwAQ8K_M}*g;@-kBy7btHVC|cZ6JoEM#%C4w&ch%!89Rh zo9^vv()YTME>P0OeJyPX4K@vJNVgwJB6;7__vzfT z&Y77r^Us_sJ$dorM*u9BTU@vqpUc9v_`DWh(Bg|OdQ|3EnDo8ktLbp(3(?gSvtj~MYcBGTI4t-S9|iD;&DnoSre}o)0{G0`)4?1rVY3_ z#$xQE4d>3BwP9>*%r?f-HpWs1;E-8PneCJU7k1zmF1bny9a2PowzbyX&|clt&|V>s zYC%p-G%^qmM&jFpy@O#A7(16bYpM2}N$pvtJ!b_aEv?Og#trSwwN>kDo7=0~T5H>@ znwzWI+Usi@H?(eSuMac?TDeJp4EZ2%wuhpTc=$kEP*xv`_ATxV4#dN;1AV=V_Xb1z z2BMM0p=d0;xF@MDV)y(c1=QNK61KWL6c`Um4J@mM(6$5A&&*{&Y!>2_!vkBw zv6}8631qJBiS)$R3aUz9GSgG?TDDzauZebW)4ckgNVsvZZ%;VZYO&$1kA{N1+k>$l zZ6`YI@$Q}h!RmDT?p3u_ljD}?U@R2g#AbCg8Xt(qg8lvB7&T-M#Z~2oYH`U+mtj|R zAfPN~Ptvc;Y_-ILF_LtejruSN=XQkS;ZVFb5{h>8M7jhEO4FXP@|m2hJl;AOiS1Y-lbCYh7($qZr;F9pJ@nNsmg(xU?dvpp-J@|q!CcYYJU@Xi-%AhHQX$Wae zHdQ&)ZH%UGI>S)ko@g(Lm^~_9OS0$2D{**PWsq8DFcgo*h9;PF_s7EfO^Y$nCfCA_ zSTqzK7zop%bGm~An_8M0>!Pu~AYG;-tu&qHti>k1s*uu%##W_0g2al3U_8{Vn5br^ zhY<`>a~0`CopLY2M5vpNO`A4Jy)+t=cX!AfQi#Wf%x=N(%NN=5px$7lYcYi)o}{(l zDRuCF(VFy7b97a*&%-hmN{4U%t!=IF&DdS+%aq z>JCOadc!7~*-HaujA^#Mf!m0Bq3&>KACYd^(v*gRY+fwfAC1L#gnMEg+iF^tGr}fd zFjehl%!&{7QzUmdGC;?yj}qr-*!0f92o2iN$MD!U*hi4-ipJxrJeL)U>UuQ3&b@qW zTBJDXMqK8)5yP!+bfd?Oz1ZgvpIfe$d2aNg&y5H=-H2+@ud~kJAvZpy#fTQ8EaV!z z-i-lHk!x9#5-sLyaUEXokW#mliC=JCnxrE&qPS&&lsjahTNcS;!haIa15M*WL@>+j zO;J|$U{9|V0=7lM2l|QG;SQhTfp5tHw=9tghb(o=GNGy~eVx(X-so+#Y!24et?~_Y zM+bX5d|lx~e2 zZcx!yrqRgW^qGZVdQvZzk0v9a=TgPEO{`=a7I~HpymseVScNZEDJ4F|Oujg-+e8o+fdnwH6Q6L57vo#rNP|htM=O zNuyF~a>y39Y?W6Kh27GOVYjqMt6R3Qh7&lc#e;6yt^+$TOq?K)x$)olD66qSI;=g| zI}mo`9kSRhH%S{imR2lZ=Ei9?JNvTb6-1J`NyTWaWTw7992;O}mEs+9wh=A*6=CQE z#FvH3ePJ`fjxebCN(mBWJ}YGmcQ7q-GY#J*NHHBNu%|lTMA|RlMd7m7hpEB=g)3Do zONRid*A60lkPaGBr3adlmkWDonO-mKdW?(_d{nGxudW@h)+egVmX7zbs=W)+@H-gq z>0P|t(%?4MnUKc2W6|3Jya2@_!QSRD6QW2}WQZP@OfF5>u~bBd$sP!&WZvazgNWKr zGErWoD#=ft<#_JPbBIxHo?uT`B;28%YU+=HCQ~mJdX-u@Rj_zbq)b2}S+sUB*;cu_ zH1ExhM>mEKSm2@IhsmbNOA4lPr86E)PMIve)vMwYHIpLoboTVbHN|J=CdL=dX$cUe zdYySOc~yYJ9Mf8DTFs@}$101Ic1@V&s^pc)KXIW*9iDpo?4E(@iQ6a5l)=01|ACR6 zW!oKcvtUsg(^F%pvr^h?6FDvQFJ)C zD*Y{8cfZ_XYUl$m-ApGmWW}+x3H{4a&SlwC#k_$-4uM zHBF5*_1jtk+ZEiI*qQfB6Au{*oQ+NG4OOi*8wofqP4$8G$(CzJZPn&vBQwM>S;b^o zbMqQL6kf-Hhp;+7%>kx35i0nu@n*hbb@Qd>EqEp0skO6ZU^m;rl=dFBLn-YJT!kt)TFkc1ClhY7pEtF|8@?n90 z6ymp^g5h6q3O0ZFDcJoBMv!p|nSQ@*=P5W&!Ko9n9yE#F#&RBAL|2*YIQjs#2Z=j;p}yM!fCmdID8znI+k zi)Yw(D@(RXEX^k)Ku3b5Fj?v(OI>`6+>K&xTEdHopZZd^mRs9Q$<`sxF-!*UBn4AL zxQjJAnPmFNF>24a%F>?9wypL%M&LS(e*Z&c@F6&sd1fC-*6q8O7pc6Ya8dK8(So; zRfYG#brQJ^!?>BN@;*u#fkuAyIPCn+<##^6+PBi0xs)@F!^k=b&oDAiBA=`0jQ4rX zzG>`poyPP>G2;Y>Y~>@ES$={vY*16_q+Np=H1H<4@MbjN?bwEUuoLg0ksRg;-N&2c zduf>;z@0dPqxc{`f@An76WmX*JcZ-<9zKp|+4>1Si9a(N{wp37J5GuN4@oXYWhOIS zJ=dFQ0}Z$iujQ@=DY#N9(*0M@D0qtH&KOWu#rF3G~qiumhYQtooO+Sx8Xmy&cXh-Q>_A3 zh!eK+@W_%SDPjwc_g+hExt%lAxPGXnF@kyZgqCYQ#twd-;8z_?vE*8QJ&C7}&tsf# zZzz8p8(4Aj>*!cpobSW^n0p2#RMGr4cA5@XOp@1)qI48xC(gh>-meQsv4G}zovzpB zF%+Faxz4bavoM!y|0otVoWY{Da*Nbr(ph58;-EcsBI_!}2r5Rg^n_{k%O-5S+7#he z6zJFd&FMK*;*b1Y=})M^Ic&mTsI*^d@I00GFIxI}TJ;#7qzgXH=QCoM3infG zHMkz{;0hb548a0Rsxe`oPR>`0whmzyGS6cPZSMjq9r#aq0W+_mftE}=(U8(wV%*gCm;Vf_v3?9Jhlej)HuVSHAZdjRh z1~-~&D&%B)c|jH*<2s$8rbaN6A8_xKJ$F{NXGI zi^PK)B_CDdMHij%HkpMx_*6IVB==Q>s(0btCg>j;2l7J%6}3#tMrgUuwA(`jmv@_X zb_nmo`%P;+M6l^(5XdCGBdldILLPsYLQSCcl?R&PUmtVkUmXe~>ES}&o zOQ}XTx1ga*1>N%%!o8`rbtl-UQfK#^JBp?$Mm+R+POR36m#*^Ay)QSm+D~ApSyS{r zYnvj^tB&GY|7omG>6&*0Sw)^Zj_Q=!6S_%}XV;6T-C|DTKw_Fst!pgu%sYyVgZ)5<^NgU^7zu06*vgDxT%k45t?vS~1rs`I+@t$FrONJDjAB$4y`Hj_>N#!ktgQ}@{2sK zBWm?ojL=0Ip-taZMl63G9(&59tqG2$u@4Zvj$n=)g-?!AWA|f$e3%ydfXSDS8)fpp z3D(I^){|uy$}3|j1HxRQ-~_{lzBacQ<|)`rhMy?30;paF>=(ya0=vg9n$Ifzn*nPJ z^q&TN?yyq(HiEf>S+m2>Y(CF^+&{TPb7_OU-iA>$4#VyDHklP$A=Az^Mcyq(QD82~ zFlq2^kZ01hA20H5onp))SLkux?rlyEXPQgx6H9FqXL*oXgq%jXJd6rqypYdeoqQJ6 zau)USD7MPyuw6cno8*glrF;oNd7M$<3G9`xA}U|QpgcuS|2poLZxErsiMPnpxL2N` zr+*tqrqYbA_Z`Je3 zfwO$Z45#xv&$w{Tq(kD*8VRd9!KdbV8z}w3`-p;+*cit0LT}3+I{GdDCGtDu$?q{!o}<0}0VVQBM$12;QqEzm{Dpe^ z2WsV?rd(F$wqabZ4GFna8V54+a`W<1j?zN!c9U^Sjv1!2ObDWk zsyWyv4%2do^JMe_8NEmt8N*ydaJ7+v`G$sEI=88#(s9O0%}m|OW-?$?whVoJ%~F|Z zCAz3meP^&^JUzaN87`r&$lI1ob$6cS9K*bJWGWIEo|MgeLu!&6*US`){s1keq zn8>79*2MJ1N<7yT6&^#CrZ|toX{^<|*+g2XQ!*dXxg8_ewYHd`t2&{7Ig4rW&Z6So zYlg=jP0^2ekVX+)MltdXA7&a?qtLhpl|~6x8uPK*C`Fx7hE0Ya4aNdf1clt$G=$HX zkXy;WHCZ*XIWATcKFd+QrbjIv88_NIWSr+enmig@GGAbVIw7?IOJ9#NeuB#c|Bx!F zSy%8F{!Zu8cLxewc4n(F{!*v6n&+8`c5&j{c}xB{Yc?8`b=xVR`aZ!xV9;rRpOIh1 z)`8Q~++cZv$Ml49&#m^>VKf(d8);P@(;dn^G3{Gh=xt)(%yQ3wz18pCG>SmEC$1yc z7J9c>BZE4!(Hz;YBQ9IH=Qew5!dc2a2UzE}HeS^=+EDb_4r!fMP>{(VBEf%!Wy4O4 z$R2#^L>IB4SAJruKuy(7p^_?yCRC6r_$Mj2q6L^? zT7s`L1@~?>g;dum_Y5h+a})lPl>A^)idU%=rYDtqZcmlMWoDq1V!9=Rx|dykNf~}c z8GcPH`VFJ=@AxM^UN(pWdTbWYtH*g>%`1lx z&?{B3x0S^Re52S_?zzL>Y8i{FQJu>!&!=ovGQL#_PBN{L6uYo9geB}k+zfJ>r5w~v z)oJ+U&92;Y$lh8^FS~_pdReZq!d~vV(>#0a;aBe!lYDJEr{v=Uq{1B7IS`mff!*ov{z;?AN9$){Be_m`26QOhxpZ7wBQ*MnbHbMy#4`WD&dg_d>&s z%|<(PK`X9;|^>9YH@bhWhEr%889TSh+aJ}vuNc8*AN qhi&Pj(%SYZY14FC-&C2WG)U=SNF`h9-oKZnvV2_uQC?y4^uGbhLXS59 literal 0 HcmV?d00001