Byte code instrumentation fix #449

dhaval24 · 2017-10-20T23:52:06Z

This pull request fixes the java.lang.Verify error which was being caused in JDK version 1.7 onwards because of improper instrumentation. Changes here facilitate the creation of StackMap frame (a datastructure introduced in JDK 1.7 and mandated in 1.8), fixes constructor instrumentation issue along with improper constructor signature for SQLServer.

#276 mentions issues with HTTPCLIENT 42, however on current testing things work fine for HTTPCLIENT 43 and since its more secure it is advisable to upgrade to this client.

This pull request also upgrades the ASM version to ASM 5.2

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public surface reviewed
CHANGELOG.md updated

…soft/ApplicationInsights-Java into ByteCodeInstrumentationFix

…cle and sqlserver instrumentation

Dmitry-Matveev · 2017-10-21T01:52:23Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/AgentImplementation.java

@@ -153,6 +153,8 @@ public static String getAgentJarLocation() throws UnsupportedEncodingException {
        }

        String path = AgentImplementation.class.getProtectionDomain().getCodeSource().getLocation().getPath();
+        int index = path.lastIndexOf('/');


Is it always in this notation across the platforms or sometimes can be '\'?

@Dmitry-Matveev It's a good question. I have an idea for 2 platforms I have tested so far. Java 8 has by default class loader of URLClassLoader type so it won't hit this part of the class .Java 9 has default classloader called "app" which is not of the type URLClassLoader and hence hits this part and only works for this change. I am not sure about other JVM's though.

Dmitry-Matveev · 2017-10-21T01:56:13Z

.../src/main/java/com/microsoft/applicationinsights/agent/internal/agent/CustomClassWriter.java

+        do {
+            class1 = class1.getSuperclass();
+        }
+        while (!(class1.isAssignableFrom(class2)));


Any possible issue with endless cycle or exception here if class1 has no more superClass and class2 happens to be not assignable (e.g. null or something...)?

@Dmitry-Matveev to be specific about this, this is the exact implementation as how ASM does by default. Only reason to override this method was because ASM uses Class.getClassLoader() method to load the current class and therefore it gets the ClassLoader which doesn't have all the required classes loaded in it. In this override I pass the ClassLoader from the Callee in order to avoid the lookup and have the classloader which has all the required classes loaded. Therefore I believe that since there is minimal change done from ASM implementation this possibility is very narrow. Is there a way we can test this? How can we know if there is a null being passed or something like that?

@dhaval24 To test this method, you could refactor it into a public static utility method which takes the ClassLoader along with the other parameters and write tests against that.

Dmitry-Matveev · 2017-10-21T02:00:30Z

...crosoft/applicationinsights/agent/internal/agent/sql/PreparedStatementClassDataProvider.java

@@ -243,7 +251,8 @@ private ClassVisitorFactory classFactoryForSqlServer() {
            @Override
            public ClassVisitor create(ClassInstrumentationData classInstrumentationData, ClassWriter classWriter) {
                HashSet<String> ctorSignatures = new HashSet<String>();
-                ctorSignatures.add("(Lcom/microsoft/sqlserver/jdbc/SQLServerConnection;Ljava/lang/String;II)V");
+                ctorSignatures.add("(Lcom/microsoft/sqlserver/jdbc/SQLServerConnection;Ljava/lang/String;" +
+                        "IILcom/microsoft/sqlserver/jdbc/SQLServerStatementColumnEncryptionSetting;)V");


Is it the same parameter across all SQL calls we'd need to monitor or specific to some version? The previous signature looked more generic.

So the default signature of the constructor for JDBC driver version 6.2 is the one I changed. With the previous signature we were missing the constructor instrumentation. Now with the older drivers it might well be possible that signature was one mentioned before but nonetheless I believe enterprise can upgrade to latest JDBC drivers.

@dhaval24 Does this limit the supported driver versions?

Dmitry-Matveev · 2017-10-21T02:01:25Z

...a/com/microsoft/applicationinsights/agent/internal/agent/DefaultByteCodeTransformerTest.java

-        byte[] result = tested.transform(mockArray, "mock");
-        assertSame(result, mockArray);
-    }
+//    @Test


Should we delete the test if no longer relevant or uncomment and fix if it still is?

…thout assert in ClassInstrumentationDataTest and removing unused imports

dhaval24 · 2017-10-21T04:18:39Z

@Dmitry-Matveev I have fixed the test for DefaultByteCodeTransformerTest and commented the tests which do not have any asserts. They just add to build time as they are not verifying anything for now.

SergeyKanzhelev · 2017-10-23T18:13:49Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/AgentImplementation.java

@@ -153,6 +153,8 @@ public static String getAgentJarLocation() throws UnsupportedEncodingException {
        }

        String path = AgentImplementation.class.getProtectionDomain().getCodeSource().getLocation().getPath();


Use https://docs.oracle.com/javase/8/docs/api/java/nio/file/Paths.html instead of string?

SergeyKanzhelev · 2017-10-23T18:14:21Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/AgentImplementation.java

@@ -153,6 +153,8 @@ public static String getAgentJarLocation() throws UnsupportedEncodingException {
        }

        String path = AgentImplementation.class.getProtectionDomain().getCodeSource().getLocation().getPath();
+        int index = path.lastIndexOf('/');
+        path = path.substring(0, index + 1);


check that index is not -1. Just in case so code will not throw. Also see above - using Path instead of string will help

SergeyKanzhelev · 2017-10-23T18:14:49Z

.../src/main/java/com/microsoft/applicationinsights/agent/internal/agent/CustomClassWriter.java

+
+import org.objectweb.asm.ClassWriter;
+
+public class CustomClassWriter extends ClassWriter {


Please add comment. And make sure you need it to be public

SergeyKanzhelev · 2017-10-23T18:15:12Z

.../src/main/java/com/microsoft/applicationinsights/agent/internal/agent/CustomClassWriter.java

+        this.classLoader = loader;
+    }
+
+    protected String getCommonSuperClass(String className1, String className2)


Add comment for this method.

SergeyKanzhelev · 2017-10-23T18:16:02Z

...ava/com/microsoft/applicationinsights/agent/internal/agent/ClassInstrumentationDataTest.java

-    private final static String MOCK_SIGNATURE_1 = "Signature1";
-    private final static String MOCK_SIGNATURE_2 = "Signature2";
-    private final static String MOCK_SIGNATURE_3 = "Signature3";
+//    private final static String MOCK_METHOD = "Method";


remove instead of commenting. BTW, why commenting?

Commenting because this signature was used in tests which were relying on getDecisionMethod() however it turns out that there is no such method in ClassInstrumentationData class.

May be if there is an idea to get this tests running in future, that was the reason.

What happens if you uncomment these tests?

Please address these tests. If they are failing, they should be updated to reflect the new functionality. If they are no longer valid, they should be removed (and explain why this is the case).

@littleaj all the asserts within this tests are commented because there is a method which they intended to write is missing. Therefore there is no use to create unnecessary objects during test run time. It will add overhead only.

@dhaval24 as discussed, let's just revert this to the master copy and we'll come back to it later. Then, we'll figure out if it needs to be removed or modified so it works.

SergeyKanzhelev · 2017-10-23T18:16:33Z

gradle/common-java.gradle

@@ -40,8 +40,8 @@ if (java6JreDir) {
        bootClasspath += "$archivePath;"
    }
    tasks.withType(JavaCompile) {
-        sourceCompatibility = 1.6
-        targetCompatibility = 1.6
+        sourceCompatibility = 1.8


So you dropped 1.7 support?

I made it so because sometimes in some situations it the 1.7 target code would not produce the desired exceptions because these changes were mandated from JDK 1.8. Also, we need to decide till what point do we need to support backward compatibility in the SDK. We might not be able to use all the new features of JDK 8 like automatic inference of types in <> operator, passing of functions using lambda expressions etc which is modern java.

If required no harm in reverting it back if things work fine. However at one point we need to make this decision.

SergeyKanzhelev · 2017-10-23T18:17:03Z

...ain/java/com/microsoft/applicationinsights/internal/agent/CoreAgentNotificationsHandler.java

-        return explainSB;
-    }
-}
+/*


what changed in this file?

@grlima to comment more on this. At-least my intuition here was that while sending telemetry for SqlServer there are only two arguments that come in the methoddata object whereas previously there was a strict condition which checked arguments should be more than or equal to 3

@dhaval24 can you fix the diff here? It will be better for the history to have only the changes in the diff. Glancing at the first 100 lines, I couldn't see any changes.

@littleaj this file was edited by @grlima when prepared statements for sqlserver where not working (i.e not capturing the query data) we can no revert this changes back because we no longer need to loose if condition in the sendSQLTelemetry() method.

you can see the difference here: git diff master -- CoreAgentNotificationsHandler.java > CoreAgentNotificationsHandler.java.diff (assuming PWD is the file's location). It's the line endings. Your IDE should be set to preserve the existing line ending characters. There may be a way to fix this in IntelliJ, but you may have to load it in Notepad++ an do a find/replace.

If you want to just checkout a clean copy and make the changes again, add use this command to see the "actual" changes: git diff master -w -- CoreAgentNotificationsHandler.java > CoreAgentNotificationsHandler.java.diff to ignore all whitespace changes

…ng to 1.7

dhaval24 · 2017-10-23T23:17:17Z

@SergeyKanzhelev I have addressed most of the concerns including using the Paths class for locating the agent jar, adding comments and also making CustomClassWriter package private instead of public.

littleaj · 2017-10-25T17:28:00Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/AgentImplementation.java

+        if (stringPath.charAt(0) == '/') {
+            stringPath = stringPath.substring(1);
+        }
+        Path path = Paths.get(stringPath);


The java.nio.* classes were introduced in 1.7. It's my understanding we still want compatibility with 1.6. If that's still the case, this needs to be reworked. @grlima, can you confirm?

littleaj

I'd like to see tests which validate your changes.

littleaj · 2017-10-25T20:40:17Z

...ain/java/com/microsoft/applicationinsights/internal/agent/CoreAgentNotificationsHandler.java

-        return explainSB;
-    }
-}
+/*


@dhaval24 can you fix the diff here? It will be better for the history to have only the changes in the diff. Glancing at the first 100 lines, I couldn't see any changes.

littleaj · 2017-10-25T20:41:21Z

gradle/common-java.gradle

@@ -40,8 +40,8 @@ if (java6JreDir) {
        bootClasspath += "$archivePath;"
    }
    tasks.withType(JavaCompile) {
-        sourceCompatibility = 1.6


This could be a breaking change for some users. I think we should wait until we have a more comprehensive plan before changing compatibility.

littleaj · 2017-10-25T20:44:30Z

...a/com/microsoft/applicationinsights/agent/internal/agent/DefaultByteCodeTransformerTest.java

@@ -27,10 +27,10 @@

 public final class DefaultByteCodeTransformerTest {
    @Test
-    public void noClassInstrumentationDataTest() {
+    public void noClassInstrumentationDataTest() throws ClassNotFoundException {


It doesn't look like you need throws here. transform doesn't throw anything. Test methods only need throws declarations to satisfy the compiler.

sure this is by mistake. I think there is any need of throws too.

littleaj · 2017-10-25T21:38:46Z

.../src/main/java/com/microsoft/applicationinsights/agent/internal/agent/CustomClassWriter.java

+        do {
+            class1 = class1.getSuperclass();
+        }
+        while (!(class1.isAssignableFrom(class2)));


@dhaval24 To test this method, you could refactor it into a public static utility method which takes the ClassLoader along with the other parameters and write tests against that.

littleaj · 2017-10-25T21:39:21Z

...ava/com/microsoft/applicationinsights/agent/internal/agent/ClassInstrumentationDataTest.java

-    private final static String MOCK_SIGNATURE_1 = "Signature1";
-    private final static String MOCK_SIGNATURE_2 = "Signature2";
-    private final static String MOCK_SIGNATURE_3 = "Signature3";
+//    private final static String MOCK_METHOD = "Method";


What happens if you uncomment these tests?

littleaj · 2017-10-25T21:50:13Z

...crosoft/applicationinsights/agent/internal/agent/sql/PreparedStatementClassDataProvider.java

@@ -243,7 +251,8 @@ private ClassVisitorFactory classFactoryForSqlServer() {
            @Override
            public ClassVisitor create(ClassInstrumentationData classInstrumentationData, ClassWriter classWriter) {
                HashSet<String> ctorSignatures = new HashSet<String>();
-                ctorSignatures.add("(Lcom/microsoft/sqlserver/jdbc/SQLServerConnection;Ljava/lang/String;II)V");
+                ctorSignatures.add("(Lcom/microsoft/sqlserver/jdbc/SQLServerConnection;Ljava/lang/String;" +
+                        "IILcom/microsoft/sqlserver/jdbc/SQLServerStatementColumnEncryptionSetting;)V");


@dhaval24 Does this limit the supported driver versions?

…boot

…me, reverting Path API use in Agent Impl, adding public parameterized constructor for spring boot support, updating build script for web allows shadow jar

dhaval24 · 2017-11-16T19:22:43Z

@SergeyKanzhelev @littleaj @Dmitry-Matveev @grlima I have pushed changes in this PR which are now addressing all the possible bytecode concerns we have seen till the date. As far as unit tests are concerned I still do not have a way to perform tests and our agent code does not have ones till the date, however this is a blocking issue and we need to get it fixed. I have done end to end testing in several test application and changes work fine. The CustomClass Loader now performs the commonSuperClass lookup without actually loading the class. The PR also introduces changes in the Application-Insights web gradle file to include shadow jar plugin hence eliminating class path hell issues and also introduces a public constructor in the WebRequestTracking filter to address SpringBoot solutions as discussed.

littleaj

I haven't seen anything addressed from my last review. Let me know if you have any questions about my comments.

littleaj · 2017-11-17T00:34:39Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/AgentImplementation.java

                }
            }
        } catch (Throwable throwable) {
+            InternalAgentLogger.INSTANCE.logAlways(InternalAgentLogger.LoggingLevel.ERROR,"Unable to find the Agent Jar");


Remove this extra log message. It's going to the same logger, same level and there's no new information here. Feel free to update the wording in the log message on the next line.

okay thanks will do this.

littleaj · 2017-11-17T00:36:03Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/CustomClassWriterv2.java

+     * @return The String for the common super class of both the classes
+     */
+    @Override
+    protected String getCommonSuperClass(final String type1, final String type2) {


At the very least this method should have a unit test, if not the whole class.

Sure I will get couple of them.

littleaj · 2017-11-17T00:38:54Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/CustomClassWriterv2.java

+ * provided by DefaultByteCode transformer (This loader essentially has all
+ * the required classes already loaded)
+ */
+public class CustomClassWriterv2 extends org.objectweb.asm.ClassWriter {


I'm not a fan of SomeClassV2 naming schemes. If the modifications can't be described in one or two words, the go with "branding" it; e.g. AppInsightsCustomClassWriter.

I think this can be just refactored as CustomClassWriter I will do that.

littleaj · 2017-11-17T00:44:29Z

...rc/main/java/com/microsoft/applicationinsights/agent/internal/agent/DefaultClassVisitor.java

@@ -51,7 +52,7 @@ public void visit(int version, int access, String name, String signature, String
    @Override
    public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) {
        MethodVisitor originalMV = super.visitMethod(access, name, desc, signature, exceptions);
-
+        originalMV = new JSRInlinerAdapter(originalMV, access, name, desc, signature, exceptions);


The next line checks for null, which will now always be false. Either that part of the condition is no longer valid and should be removed, or it should be relocated.

well yes I think can be moved one step above may be.

littleaj · 2017-11-17T00:47:00Z

...ava/com/microsoft/applicationinsights/agent/internal/agent/ClassInstrumentationDataTest.java

-    private final static String MOCK_SIGNATURE_1 = "Signature1";
-    private final static String MOCK_SIGNATURE_2 = "Signature2";
-    private final static String MOCK_SIGNATURE_3 = "Signature3";
+//    private final static String MOCK_METHOD = "Method";


Please address these tests. If they are failing, they should be updated to reflect the new functionality. If they are no longer valid, they should be removed (and explain why this is the case).

littleaj · 2017-11-17T00:48:13Z

gradle/common-java.gradle

@@ -40,8 +40,8 @@ if (java6JreDir) {
        bootClasspath += "$archivePath;"
    }
    tasks.withType(JavaCompile) {
-        sourceCompatibility = 1.6


…in one test case, refractoring name, removing unnecessary null check

dhaval24 · 2017-11-17T19:35:32Z

@littleaj I have addressed your concerns in the previous review except the diff in CoreAgentNotification Handler. In my IDE git diff shows no difference so will have to figure it out. @grlima had pushed those changes so if you can have a look @grlima would be great.

LGTM

dhaval24 · 2017-11-20T22:26:01Z

Thanks Sergey for your comment. @littleaj and @grlima I am waiting for your approval to merge this into master. Please have a look.

dhaval24 · 2017-11-21T01:23:52Z

Folks I have updated the last missing piece on my end which was updating changelog file. If there is anything else which is left let me know so we that we can then merge this.

dhaval24 and others added 8 commits October 15, 2017 23:13

adding facility to recompute stack frames

e1d4ecb

Making changes to fix the instrumentation issues

ccee2e7

Merge branch 'master' into ByteCodeInstrumentationFix

2045f39

Renaming class writer to more meaningful name

38859c9

fix for SQL dependency reporting (do no assume 3 arguments)

7852ed5

Merge branch 'ByteCodeInstrumentationFix' of https://github.com/Micro…

f63266a

…soft/ApplicationInsights-Java into ByteCodeInstrumentationFix

sendSQLTelemetry: add check for arguments

68305d9

Removing trycatch instrumentation before base constructor - fixes ora…

10bb456

…cle and sqlserver instrumentation

dhaval24 requested review from littleaj, SergeyKanzhelev, Dmitry-Matveev and grlima October 20, 2017 23:52

Dmitry-Matveev reviewed Oct 21, 2017

View reviewed changes

modifying test of DefaultByteCodeTransformerTest, commenting tests wi…

0e2f39a

…thout assert in ClassInstrumentationDataTest and removing unused imports

SergeyKanzhelev previously requested changes Oct 23, 2017

View reviewed changes

using Paths class for locating agent jar, adding comments and reverti…

f71254e

…ng to 1.7

littleaj reviewed Oct 25, 2017

View reviewed changes

littleaj suggested changes Oct 25, 2017

View reviewed changes

dhaval24 added 2 commits November 10, 2017 14:35

fixing duplicate class definition issue and temp solution for spring …

e13546b

…boot

Adding new custom class writer to find superclass without using forna…

7449b25

…me, reverting Path API use in Agent Impl, adding public parameterized constructor for spring boot support, updating build script for web allows shadow jar

littleaj suggested changes Nov 17, 2017

View reviewed changes

dhaval24 added this to the 1.0.11 milestone Nov 17, 2017

dhaval24 self-assigned this Nov 17, 2017

Addressing feedback, adding test for customclasswriter, fixing throw …

3f2d68d

…in one test case, refractoring name, removing unnecessary null check

dhaval24 added 2 commits November 17, 2017 15:02

Removing unused import

b30ce49

changing the line ending

08cd2ee

updating changelog

d81d9ed

dhaval24 added 3 commits November 27, 2017 11:03

Removing superflous logging statement

e5931d7

Uncommenting tests will be addressed seperately

79e3a23

Merge branch 'master' into ByteCodeInstrumentationFix

75596d8

littleaj approved these changes Nov 27, 2017

View reviewed changes

dhaval24 merged commit d4ae36c into master Nov 27, 2017

dhaval24 modified the milestones: 1.0.11, 2.0.0 Dec 27, 2017

dhaval24 deleted the ByteCodeInstrumentationFix branch March 14, 2018 15:11

		@@ -153,6 +153,8 @@ public static String getAgentJarLocation() throws UnsupportedEncodingException {
		}

		String path = AgentImplementation.class.getProtectionDomain().getCodeSource().getLocation().getPath();


		import org.objectweb.asm.ClassWriter;

		public class CustomClassWriter extends ClassWriter {

Byte code instrumentation fix #449

Byte code instrumentation fix #449

Conversation

dhaval24 commented Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 commented Oct 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 commented Oct 23, 2017

littleaj Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

littleaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 commented Nov 16, 2017

littleaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 commented Nov 17, 2017

dhaval24 commented Nov 20, 2017

dhaval24 commented Nov 21, 2017

dhaval24 commented Oct 20, 2017 •

edited

Loading

littleaj Oct 25, 2017 •

edited

Loading