diff --git a/class-inject/src/glue/java/datadog/instrument/glue/DefineClassGlueGenerator.java b/class-inject/src/glue/java/datadog/instrument/glue/DefineClassGlueGenerator.java index 06c523b..224ecac 100644 --- a/class-inject/src/glue/java/datadog/instrument/glue/DefineClassGlueGenerator.java +++ b/class-inject/src/glue/java/datadog/instrument/glue/DefineClassGlueGenerator.java @@ -25,8 +25,8 @@ /** * Generates glue bytecode for a {@link BiFunction} around {@code ClassLoader.defineClass} that * accepts a map of class-names to bytecode and a class-loader (or protection domain) and returns - * the defined classes. The glue bytecode is encoded as a UTF-16BE string and written as a constant - * inside a Java class at build-time. + * the defined classes. The glue bytecode is encoded as an ISO-8859-1 string and written as a + * constant inside a Java class at build-time. * *

At runtime the glue bytecode is decoded from the string and defined as a hidden/anonymous * class using instrumentation. This class provides access to {@code ClassLoader.defineClass} @@ -414,11 +414,6 @@ static byte[] generateBytecode(String unsafeNamespace) { mv.visitMaxs(-1, -1); mv.visitEnd(); - // pad bytecode to even number of bytes, to make string encoding/decoding easier - if ((unsafeNamespace.length() & 0x01) == 0) { - cw.newConst(0); - } - cw.visitEnd(); return cw.toByteArray(); diff --git a/class-inject/src/main/java/datadog/instrument/classinject/ClassInjector.java b/class-inject/src/main/java/datadog/instrument/classinject/ClassInjector.java index c51390f..3c29462 100644 --- a/class-inject/src/main/java/datadog/instrument/classinject/ClassInjector.java +++ b/class-inject/src/main/java/datadog/instrument/classinject/ClassInjector.java @@ -290,9 +290,12 @@ public void visitCode() { mv.visitLdcInsn(DefineClassGlue.V8); } - // unpack the UTF-16BE encoded string back into bytecode + // unpack the ISO-8859-1 encoded glue string back into bytecode mv.visitFieldInsn( - GETSTATIC, "java/nio/charset/StandardCharsets", "UTF_16BE", "Ljava/nio/charset/Charset;"); + GETSTATIC, + "java/nio/charset/StandardCharsets", + "ISO_8859_1", + "Ljava/nio/charset/Charset;"); mv.visitMethodInsn( INVOKEVIRTUAL, "java/lang/String", "getBytes", "(Ljava/nio/charset/Charset;)[B", false); diff --git a/class-inject/src/test/java/datadog/instrument/glue/DefineClassGlueGeneratorTest.java b/class-inject/src/test/java/datadog/instrument/glue/DefineClassGlueGeneratorTest.java index a68e0cc..d234c3f 100644 --- a/class-inject/src/test/java/datadog/instrument/glue/DefineClassGlueGeneratorTest.java +++ b/class-inject/src/test/java/datadog/instrument/glue/DefineClassGlueGeneratorTest.java @@ -1,5 +1,6 @@ package datadog.instrument.glue; +import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.assertj.core.api.Assertions.assertThat; import java.io.PrintWriter; @@ -22,4 +23,18 @@ void generatedBytecodePassesVerification(String unsafeNamespace) { assertThat(errors.toString()).isEmpty(); } + + /** ClassInjector inlines the glue unpacking, so check the packed glue decodes as expected. */ + @ParameterizedTest + @ValueSource(strings = {"sun/misc", "jdk/internal/misc"}) + void packedGlueUnpacksToGeneratedBytecode(String unsafeNamespace) { + byte[] originalBytecode = DefineClassGlueGenerator.generateBytecode(unsafeNamespace); + String packedBytecode = + "sun/misc".equals(unsafeNamespace) ? DefineClassGlue.V8 : DefineClassGlue.V9; + + // mirrors exactly what ClassInjector's generated bytecode does at runtime + byte[] decodedBytecode = packedBytecode.getBytes(ISO_8859_1); + + assertThat(decodedBytecode).isEqualTo(originalBytecode); + } } diff --git a/utils/src/main/java/datadog/instrument/glue/Glue.java b/utils/src/main/java/datadog/instrument/glue/Glue.java index 65c3824..afd1270 100644 --- a/utils/src/main/java/datadog/instrument/glue/Glue.java +++ b/utils/src/main/java/datadog/instrument/glue/Glue.java @@ -6,7 +6,7 @@ package datadog.instrument.glue; -import static java.nio.charset.StandardCharsets.UTF_16BE; +import static java.nio.charset.StandardCharsets.ISO_8859_1; import static java.util.Objects.requireNonNull; import datadog.instrument.utils.JVM; @@ -26,7 +26,7 @@ private Glue() {} * @return the unpacked bytecode */ public static byte[] unpackBytecode(String bytecode) { - return bytecode.getBytes(UTF_16BE); + return bytecode.getBytes(ISO_8859_1); } /** diff --git a/utils/src/main/java/datadog/instrument/glue/GlueGenerator.java b/utils/src/main/java/datadog/instrument/glue/GlueGenerator.java index 55a47ac..d8637a4 100644 --- a/utils/src/main/java/datadog/instrument/glue/GlueGenerator.java +++ b/utils/src/main/java/datadog/instrument/glue/GlueGenerator.java @@ -51,31 +51,20 @@ public static void classHeader(List lines, String className) { /** * Packs the given glue bytecode as a string literal and writes it out in source code form. * - *

The bytecode must be padded to have even length, as this simplifies the encoding/decoding. - * * @param lines the list collecting the source code * @param bytecode the bytecode to pack */ public static void packBytecode(List lines, byte[] bytecode) { - if ((bytecode.length & 0x01) == 1) { - throw new IllegalStateException("Bytecode length is not even; requires padding"); - } StringBuilder buf = new StringBuilder(" \""); - // encode bytes using lenient form of UTF_16BE: no BOM, allow unpaired surrogates - // this is so we can unpack it later without requiring any special/complex code - for (int i = 0; i < bytecode.length; i += 2) { + // encode bytes using ISO_8859_1, where each char maps directly to the byte with the same value + // this is so we can unpack it later with a single String.getBytes(ISO_8859_1) call + for (byte b : bytecode) { if (buf.length() > 120) { lines.add(buf + "\""); buf.setLength(0); buf.append(" + \""); } - // pack 2 bytes into one char using big-endian ordering - char c = (char) ((bytecode[i] << 8) | (0x00FF & bytecode[i + 1])); - if (c <= 0x00FF) { - buf.append(String.format("\\%03o", (int) c)); - } else { - buf.append(String.format("\\u%04x", (int) c)); - } + buf.append(String.format("\\%03o", 0x00FF & b)); } lines.add(buf + "\";"); } diff --git a/utils/src/test/java/datadog/instrument/glue/GlueGeneratorTest.java b/utils/src/test/java/datadog/instrument/glue/GlueGeneratorTest.java index 7fbff66..c1a9c48 100644 --- a/utils/src/test/java/datadog/instrument/glue/GlueGeneratorTest.java +++ b/utils/src/test/java/datadog/instrument/glue/GlueGeneratorTest.java @@ -11,7 +11,6 @@ import java.net.URL; import java.net.URLClassLoader; import java.nio.file.Files; -import java.util.ArrayList; import java.util.List; import javax.tools.JavaCompiler; import javax.tools.SimpleJavaFileObject; @@ -51,9 +50,11 @@ void glueGenerator(@TempDir File resourceDir, @TempDir File javaDir, @TempDir Fi assertTrue(glueCompilationTask.call()); String packedBytecode; + String packedAllByteValues; try (URLClassLoader cl = URLClassLoader.newInstance(new URL[] {classesDir.toURI().toURL()})) { Class testGlueClass = cl.loadClass("datadog.instrument.glue.TestGlue"); packedBytecode = (String) testGlueClass.getField("BYTECODE").get(null); + packedAllByteValues = (String) testGlueClass.getField("ALL_BYTE_VALUES").get(null); } byte[] expectedBytecode = { @@ -66,23 +67,14 @@ void glueGenerator(@TempDir File resourceDir, @TempDir File javaDir, @TempDir Fi assertArrayEquals(expectedBytecode, Files.readAllBytes(testGlueResourceFile.toPath())); assertArrayEquals(expectedBytecode, Glue.unpackBytecode(packedBytecode)); - } - - @Test - void paddingRequired() { - byte[] exampleBytecodeNeedsPadding = { - -54, -2, -70, -66, 0, 0, 0, 52, 0, 10, 10, 0, 2, 0, 3, 7, 0, 4, 12, 0, 5, 0, 6, 1, 0, 16, 106, - 97, 118, 97, 47, 108, 97, 110, 103, 47, 79, 98, 106, 101, 99, 116, 1, 0, 6, 60, 105, 110, 105, - 116, 62, 1, 0, 3, 40, 41, 86, 7, 0, 8, 1, 0, 2, 69, 103, 1, 0, 4, 67, 111, 100, 101, 0, 32, 0, - 7, 0, 2, 0, 0, 0, 0, 0, 1, 0, 0, 0, 5, 0, 6, 0, 1, 0, 9, 0, 0, 0, 17, 0, 1, 0, 1, 0, 0, 0, 5, - 42, -73, 0, 1, -79, 0, 0, 0, 0, 0, 0 - }; + // every possible byte value, with an odd length to check no padding is required + byte[] expectedAllByteValues = new byte[257]; + for (int i = 0; i < expectedAllByteValues.length; i++) { + expectedAllByteValues[i] = (byte) i; + } - assertThrows( - IllegalStateException.class, - () -> GlueGenerator.packBytecode(new ArrayList<>(), exampleBytecodeNeedsPadding), - "Bytecode length is not even; requires padding"); + assertArrayEquals(expectedAllByteValues, Glue.unpackBytecode(packedAllByteValues)); } @Test diff --git a/utils/src/test/java/datadog/instrument/glue/TestGlueGenerator.java b/utils/src/test/java/datadog/instrument/glue/TestGlueGenerator.java index 0de7a73..4a368c1 100644 --- a/utils/src/test/java/datadog/instrument/glue/TestGlueGenerator.java +++ b/utils/src/test/java/datadog/instrument/glue/TestGlueGenerator.java @@ -20,6 +20,15 @@ public class TestGlueGenerator { 0, 1, 0, 1, 0, 0, 0, 5, 42, -73, 0, 1, -79, 0, 0, 0, 0, 0, 0 }; + // every possible byte value, plus one more so the length is odd + private static final byte[] allByteValues = new byte[257]; + + static { + for (int i = 0; i < allByteValues.length; i++) { + allByteValues[i] = (byte) i; + } + } + public static void generateGlue(Path resourcePath, Path javaPath) throws IOException { Files.write(resourcePath.resolve("test.glue"), exampleBytecode); @@ -28,6 +37,8 @@ public static void generateGlue(Path resourcePath, Path javaPath) throws IOExcep classHeader(lines, "TestGlue"); lines.add(" String BYTECODE ="); packBytecode(lines, exampleBytecode); + lines.add(" String ALL_BYTE_VALUES ="); + packBytecode(lines, allByteValues); lines.add("}"); Files.write(testGlue, lines, StandardCharsets.UTF_8); }