Omit instrumenting classes version pre Java 5 (#320)

Author: felixbarny

CHANGELOG.md

@@ -4,7 +4,8 @@
 
 ## Bug Fixes
  * Update dsl-json which fixes a memory leak.
- See [ngs-doo/dsl-json#102](https://github.com/ngs-doo/dsl-json/pull/102) for details.
+ See [ngs-doo/dsl-json#102](https://github.com/ngs-doo/dsl-json/pull/102) for details. 
+ * Avoid `VerifyError`s by non instrumenting classes compiled for Java 4 or earlier
 
 # 1.0.1
 

apm-agent-core/pom.xml

@@ -132,5 +132,12 @@
             <artifactId>guava</artifactId>
             <version>27.0-android</version>
         </dependency>
+        <!--Used to test excluding old class file versions-->
+        <dependency>
+            <groupId>commons-math</groupId>
+            <artifactId>commons-math</artifactId>
+            <version>1.0</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 </project>

apm-agent-core/src/main/java/co/elastic/apm/bci/ElasticApmAgent.java

@@ -22,6 +22,7 @@
 import co.elastic.apm.bci.bytebuddy.AnnotationValueOffsetMappingFactory;
 import co.elastic.apm.bci.bytebuddy.ErrorLoggingListener;
 import co.elastic.apm.bci.bytebuddy.MatcherTimer;
+import co.elastic.apm.bci.bytebuddy.MinimumClassFileVersionValidator;
 import co.elastic.apm.bci.bytebuddy.SimpleMethodSignatureOffsetMappingFactory;
 import co.elastic.apm.bci.bytebuddy.SoftlyReferencingTypePoolCache;
 import co.elastic.apm.configuration.CoreConfiguration;
@@ -34,6 +35,7 @@
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.dynamic.DynamicType;
 import net.bytebuddy.dynamic.scaffold.MethodGraph;
 import net.bytebuddy.dynamic.scaffold.TypeValidation;
 import net.bytebuddy.matcher.ElementMatcher;
@@ -196,7 +198,14 @@ public boolean matches(MethodDescription target) {
                     }
                 }, advice.getAdviceClass().getName())
                 .include(ClassLoader.getSystemClassLoader())
-                .withExceptionHandler(PRINTING));
+                .withExceptionHandler(PRINTING))
+            .transform(new AgentBuilder.Transformer() {
+                @Override
+                public DynamicType.Builder<?> transform(DynamicType.Builder<?> builder, TypeDescription typeDescription,
+                                                        ClassLoader classLoader, JavaModule module) {
+                    return builder.visit(MinimumClassFileVersionValidator.INSTANCE);
+                }
+            });
     }
 
     private static MatcherTimer getOrCreateTimer(Class<? extends ElasticApmInstrumentation> adviceClass) {

apm-agent-core/src/main/java/co/elastic/apm/bci/bytebuddy/ErrorLoggingListener.java

@@ -7,9 +7,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -26,10 +26,15 @@
 
 public class ErrorLoggingListener extends AgentBuilder.Listener.Adapter {
 
-	private static final Logger logger = LoggerFactory.getLogger(ErrorLoggingListener.class);
+    private static final Logger logger = LoggerFactory.getLogger(ErrorLoggingListener.class);
 
-	@Override
-	public void onError(String typeName, ClassLoader classLoader, JavaModule module, boolean loaded, Throwable throwable) {
-		logger.warn("ERROR on transformation " + typeName, throwable);
-	}
+    @Override
+    public void onError(String typeName, ClassLoader classLoader, JavaModule module, boolean loaded, Throwable throwable) {
+        if (throwable instanceof MinimumClassFileVersionValidator.UnsupportedClassFileVersionException) {
+            logger.warn("{} uses an unsupported class file version (pre Java 5) and can't be instrumented. " +
+                "Consider updating to a newer version of that library.", typeName);
+        } else {
+            logger.warn("ERROR on transformation " + typeName, throwable);
+        }
+    }
 }

apm-agent-core/src/main/java/co/elastic/apm/bci/bytebuddy/MinimumClassFileVersionValidator.java

@@ -0,0 +1,61 @@
+package co.elastic.apm.bci.bytebuddy;
+
+import net.bytebuddy.ClassFileVersion;
+import net.bytebuddy.asm.AsmVisitorWrapper;
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.field.FieldList;
+import net.bytebuddy.description.method.MethodList;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation;
+import net.bytebuddy.jar.asm.ClassVisitor;
+import net.bytebuddy.pool.TypePool;
+import net.bytebuddy.utility.OpenedClassReader;
+
+public enum MinimumClassFileVersionValidator implements AsmVisitorWrapper {
+
+    INSTANCE;
+
+    private static final ClassFileVersion MINIMUM_CLASS_FILE_VERSION = ClassFileVersion.JAVA_V5;
+
+    @Override
+    public ClassVisitor wrap(TypeDescription instrumentedType, ClassVisitor classVisitor, Implementation.Context implementationContext,
+                             TypePool typePool, FieldList<FieldDescription.InDefinedShape> fields, MethodList<?> methods, int writerFlags, int readerFlags) {
+        return new ClassVisitor(OpenedClassReader.ASM_API, classVisitor) {
+            @Override
+            public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) {
+                final ClassFileVersion classFileVersion = ClassFileVersion.ofMinorMajor(version);
+                if (!classFileVersion.isAtLeast(MINIMUM_CLASS_FILE_VERSION)) {
+                    throw UnsupportedClassFileVersionException.INSTANCE;
+                }
+                super.visit(version, access, name, signature, superName, interfaces);
+            }
+        };
+    }
+
+    @Override
+    public int mergeWriter(int flags) {
+        return flags;
+    }
+
+    @Override
+    public int mergeReader(int flags) {
+        return flags;
+    }
+
+    public static class UnsupportedClassFileVersionException extends RuntimeException {
+        static final UnsupportedClassFileVersionException INSTANCE = new UnsupportedClassFileVersionException();
+
+        private UnsupportedClassFileVersionException() {
+            // singleton
+        }
+
+        /*
+         * avoids the expensive creation of the stack trace which is not needed
+         */
+        @Override
+        public synchronized Throwable fillInStackTrace()
+        {
+            return this;
+        }
+    }
+}

apm-agent-core/src/test/java/co/elastic/apm/bci/InstrumentationTest.java

@@ -19,6 +19,7 @@
  */
 package co.elastic.apm.bci;
 
+import co.elastic.apm.MockTracer;
 import co.elastic.apm.configuration.CoreConfiguration;
 import co.elastic.apm.configuration.SpyConfiguration;
 import co.elastic.apm.impl.ElasticApmTracerBuilder;
@@ -28,6 +29,7 @@
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
 import net.bytebuddy.matcher.ElementMatchers;
+import org.apache.commons.math.util.MathUtils;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 import org.stagemonitor.configuration.ConfigurationRegistry;
@@ -59,6 +61,17 @@ void testDisabled() {
         assertThat(interceptMe()).isEmpty();
     }
 
+    @Test
+    void testDontInstrumentOldClassFileVersions() {
+        ElasticApmAgent.initInstrumentation(MockTracer.create(),
+            ByteBuddyAgent.install(),
+            Collections.singletonList(new MathInstrumentation()));
+        // if the instrumentation applied, it would return 42
+        // but instrumenting old class file versions could lead to VerifyErrors in some cases and possibly some more shenanigans
+        // so we we are better off not touching Java 1.4 code at all
+        assertThat(MathUtils.sign(-42)).isEqualTo(-1);
+    }
+
     private void init(ConfigurationRegistry config) {
         ElasticApmAgent.initInstrumentation(new ElasticApmTracerBuilder()
                 .configurationRegistry(config)
@@ -92,4 +105,26 @@ public Collection<String> getInstrumentationGroupNames() {
             return Collections.singleton("test");
         }
     }
+
+    public static class MathInstrumentation extends ElasticApmInstrumentation {
+        @Advice.OnMethodExit
+        public static void onMethodExit(@Advice.Return(readOnly = false) int returnValue) {
+            returnValue = 42;
+        }
+
+        @Override
+        public ElementMatcher<? super TypeDescription> getTypeMatcher() {
+            return ElementMatchers.named("org.apache.commons.math.util.MathUtils");
+        }
+
+        @Override
+        public ElementMatcher<? super MethodDescription> getMethodMatcher() {
+            return ElementMatchers.named("sign").and(ElementMatchers.takesArguments(int.class));
+        }
+
+        @Override
+        public Collection<String> getInstrumentationGroupNames() {
+            return Collections.emptyList();
+        }
+    }
 }

docs/troubleshooting.asciidoc

@@ -150,15 +150,12 @@ Also make sure to configure your firewalls so that the host the agent runs on ca
 [[trouble-shooting-old-jdbc-drivers]]
 ==== Libraries compiled against old Java versions
 
-If you are seeing exceptions like these in your application,
+If you are seeing warning like these in your application,
 it means that you are using a library which has been compiled for a very old version of Java:
 
 ----
-java.lang.VerifyError: (class: org/apache/commons/dbcp/DelegatingStatement, method: executeQuery signature: (Ljava/lang/String;)Ljava/sql/ResultSet;) Illegal type in constant pool
-----
-
-----
-java.lang.IllegalStateException: Cannot write type to constant pool for class file version Java 2
+org.apache.commons.dbcp.DelegatingStatement uses an unsupported class file version (pre Java 5) and can't be instrumented.
+Consider updating to a newer version of that library.
 ----
 
 That mostly concerns JDBC drivers.