diff --git a/src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java b/src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java index 89451fee819..40c5b1172b5 100644 --- a/src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java +++ b/src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java @@ -399,6 +399,7 @@ public final class BufWriterImpl implements BufWriter { writeU2(cpIndex(entry)); } + // Null checks entry public void writeIndex(int bytecode, PoolEntry entry) { writeU1U2(bytecode, cpIndex(entry)); } diff --git a/src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java b/src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java index bc563482f79..b01f355adb4 100644 --- a/src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java +++ b/src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java @@ -546,8 +546,8 @@ public final class DirectCodeBuilder } private void writeShortJump(int bytecode, Label target) { + int targetBci = labelToBci(target); // implicit null check int instructionPc = curPc(); - int targetBci = labelToBci(target); // algebraic union of jump | (instructionPc, target), distinguished by null == target. int jumpOrInstructionPc; @@ -570,6 +570,7 @@ public final class DirectCodeBuilder } private void writeLongJump(int bytecode, Label target) { + Objects.requireNonNull(target); // before any write int instructionPc = curPc(); bytecodesBufWriter.writeU1(bytecode); writeLongLabelOffset(instructionPc, target); @@ -617,6 +618,17 @@ public final class DirectCodeBuilder } public void writeLookupSwitch(Label defaultTarget, List cases) { + cases = new ArrayList<>(cases); // cases may be untrusted + for (var each : cases) { + Objects.requireNonNull(each); // single null case may exist + } + cases.sort(new Comparator<>() { + @Override + public int compare(SwitchCase c1, SwitchCase c2) { + return Integer.compare(c1.caseValue(), c2.caseValue()); + } + }); + // validation end int instructionPc = curPc(); bytecodesBufWriter.writeU1(LOOKUPSWITCH); int pad = 4 - (curPc() % 4); @@ -624,13 +636,6 @@ public final class DirectCodeBuilder bytecodesBufWriter.skip(pad); // padding content can be anything writeLongLabelOffset(instructionPc, defaultTarget); bytecodesBufWriter.writeInt(cases.size()); - cases = new ArrayList<>(cases); - cases.sort(new Comparator<>() { - @Override - public int compare(SwitchCase c1, SwitchCase c2) { - return Integer.compare(c1.caseValue(), c2.caseValue()); - } - }); for (var c : cases) { bytecodesBufWriter.writeInt(c.caseValue()); var target = c.target(); @@ -639,6 +644,11 @@ public final class DirectCodeBuilder } public void writeTableSwitch(int low, int high, Label defaultTarget, List cases) { + var caseMap = new HashMap(cases.size()); // cases may be untrusted + for (var c : cases) { + caseMap.put(c.caseValue(), c.target()); + } + // validation end int instructionPc = curPc(); bytecodesBufWriter.writeU1(TABLESWITCH); int pad = 4 - (curPc() % 4); @@ -646,10 +656,6 @@ public final class DirectCodeBuilder bytecodesBufWriter.skip(pad); // padding content can be anything writeLongLabelOffset(instructionPc, defaultTarget); bytecodesBufWriter.writeIntInt(low, high); - var caseMap = new HashMap(cases.size()); - for (var c : cases) { - caseMap.put(c.caseValue(), c.target()); - } for (long l = low; l<=high; l++) { var target = caseMap.getOrDefault((int)l, defaultTarget); writeLongLabelOffset(instructionPc, target); @@ -928,6 +934,7 @@ public final class DirectCodeBuilder int slots = Util.parameterSlots(Util.methodTypeSymbol(ref.type())) + 1; writeInvokeInterface(opcode, (InterfaceMethodRefEntry) ref, slots); } else { + Util.checkKind(opcode, Opcode.Kind.INVOKE); writeInvokeNormal(opcode, ref); } return this; @@ -959,7 +966,8 @@ public final class DirectCodeBuilder @Override public CodeBuilder fieldAccess(Opcode opcode, FieldRefEntry ref) { - bytecodesBufWriter.writeIndex(opcode.bytecode(), ref); + Util.checkKind(opcode, Opcode.Kind.FIELD_ACCESS); + writeFieldAccess(opcode, ref); return this; } @@ -977,6 +985,7 @@ public final class DirectCodeBuilder @Override public CodeBuilder branch(Opcode op, Label target) { + Util.checkKind(op, Opcode.Kind.BRANCH); writeBranch(op, target); return this; } @@ -1640,6 +1649,8 @@ public final class DirectCodeBuilder @Override public CodeBuilder lookupswitch(Label defaultTarget, List cases) { + Objects.requireNonNull(defaultTarget); + // check cases when we sort them writeLookupSwitch(defaultTarget, cases); return this; } @@ -1799,6 +1810,7 @@ public final class DirectCodeBuilder @Override public CodeBuilder multianewarray(ClassEntry array, int dims) { + BytecodeHelpers.validateMultiArrayDimensions(dims); writeNewMultidimensionalArray(dims, array); return this; } @@ -1845,6 +1857,8 @@ public final class DirectCodeBuilder @Override public CodeBuilder tableswitch(int low, int high, Label defaultTarget, List cases) { + Objects.requireNonNull(defaultTarget); + // check cases when we write them writeTableSwitch(low, high, defaultTarget, cases); return this; } diff --git a/test/jdk/jdk/classfile/InstructionValidationTest.java b/test/jdk/jdk/classfile/InstructionValidationTest.java index 3e065260abf..9d5b4198adf 100644 --- a/test/jdk/jdk/classfile/InstructionValidationTest.java +++ b/test/jdk/jdk/classfile/InstructionValidationTest.java @@ -23,7 +23,7 @@ /* * @test - * @bug 8341277 + * @bug 8341277 8361102 * @summary Testing ClassFile instruction argument validation. * @run junit InstructionValidationTest */ @@ -32,24 +32,77 @@ import java.lang.classfile.*; import java.lang.classfile.constantpool.ClassEntry; import java.lang.classfile.constantpool.ConstantPoolBuilder; import java.lang.classfile.instruction.*; -import java.lang.constant.ClassDesc; -import java.lang.reflect.Parameter; +import java.util.Collections; import java.util.List; import java.util.function.Consumer; import java.util.function.ObjIntConsumer; import java.util.stream.Stream; +import helpers.TestUtil; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.function.Executable; -import static java.lang.classfile.ClassFile.ACC_STATIC; import static java.lang.constant.ConstantDescs.*; +import static helpers.TestConstants.*; import static org.junit.jupiter.api.Assertions.*; import static java.lang.classfile.Opcode.*; import static org.junit.jupiter.api.Assertions.assertThrows; class InstructionValidationTest { + @Test + void testOpcodeInCodeBuilder() { + TestUtil.runCodeHandler(cob -> { + var mref = cob.constantPool().methodRefEntry(CD_System, "exit", MTD_INT_VOID); + var fref = cob.constantPool().fieldRefEntry(CD_System, "out", CD_PrintStream); + var label = cob.newLabel(); + + // Sanity + cob.iconst_0(); + assertDoesNotThrow(() -> cob.invoke(INVOKESTATIC, mref)); + assertDoesNotThrow(() -> cob.fieldAccess(GETSTATIC, fref)); + cob.pop(); + assertDoesNotThrow(() -> cob.branch(GOTO, label)); + + // Opcode NPE + assertThrows(NullPointerException.class, () -> cob.invoke(null, mref)); + assertThrows(NullPointerException.class, () -> cob.fieldAccess(null, fref)); + assertThrows(NullPointerException.class, () -> cob.branch(null, label)); + + // Opcode IAE + assertThrows(IllegalArgumentException.class, () -> cob.invoke(IFNE, mref)); + assertThrows(IllegalArgumentException.class, () -> cob.fieldAccess(JSR, fref)); + assertThrows(IllegalArgumentException.class, () -> cob.branch(CHECKCAST, label)); + + // Wrap up + cob.labelBinding(label); + cob.return_(); + }); + } + + @Test + void testLongJump() { + TestUtil.runCodeHandler(cob -> { + assertThrows(NullPointerException.class, () -> cob.goto_w(null)); + // Ensures nothing redundant is written in case of failure + cob.return_(); + }); + } + + @Test + void testSwitch() { + TestUtil.runCodeHandler(cob -> { + assertThrows(NullPointerException.class, () -> cob.tableswitch(-1, 1, cob.startLabel(), null)); + assertThrows(NullPointerException.class, () -> cob.lookupswitch(cob.startLabel(), null)); + assertThrows(NullPointerException.class, () -> cob.tableswitch(-1, 1, cob.startLabel(), Collections.singletonList(null))); + assertThrows(NullPointerException.class, () -> cob.lookupswitch(cob.startLabel(), Collections.singletonList(null))); + assertThrows(NullPointerException.class, () -> cob.tableswitch(-1, 1, null, List.of())); + assertThrows(NullPointerException.class, () -> cob.lookupswitch(null, List.of())); + // Ensures nothing redundant is written in case of failure + cob.return_(); + }); + } + @Test void testArgumentConstant() { assertDoesNotThrow(() -> ConstantInstruction.ofArgument(SIPUSH, 0)); @@ -63,6 +116,14 @@ class InstructionValidationTest { assertThrows(IllegalArgumentException.class, () -> ConstantInstruction.ofArgument(SIPUSH, (int) Short.MAX_VALUE + 1)); assertThrows(IllegalArgumentException.class, () -> ConstantInstruction.ofArgument(BIPUSH, (int) Byte.MIN_VALUE - 1)); assertThrows(IllegalArgumentException.class, () -> ConstantInstruction.ofArgument(BIPUSH, (int) Byte.MAX_VALUE + 1)); + + TestUtil.runCodeHandler(cob -> { + assertThrows(IllegalArgumentException.class, () -> cob.sipush((int) Short.MIN_VALUE - 1)); + assertThrows(IllegalArgumentException.class, () -> cob.sipush((int) Short.MAX_VALUE + 1)); + assertThrows(IllegalArgumentException.class, () -> cob.bipush((int) Byte.MIN_VALUE - 1)); + assertThrows(IllegalArgumentException.class, () -> cob.bipush((int) Byte.MAX_VALUE + 1)); + cob.return_(); + }); } /** @@ -163,26 +224,15 @@ class InstructionValidationTest { // CodeBuilder can fail with IAE due to other reasons, so we cannot check // "success" but can ensure things fail fast static void ensureFailFast(int value, Consumer action) { - // Can fail with AssertionError instead of IAE Consumer checkedAction = cob -> { - action.accept(cob); - fail("Bad slot access did not fail fast: " + value); + assertThrows(IllegalArgumentException.class, () -> action.accept(cob)); + cob.return_(); }; - var cf = ClassFile.of(); - CodeTransform noopCodeTransform = CodeBuilder::with; - // Direct builders - assertThrows(IllegalArgumentException.class, () -> cf.build(ClassDesc.of("Test"), clb -> clb - .withMethodBody("test", MTD_void, ACC_STATIC, checkedAction))); - // Chained builders - assertThrows(IllegalArgumentException.class, () -> cf.build(ClassDesc.of("Test"), clb -> clb - .withMethodBody("test", MTD_void, ACC_STATIC, cob -> cob - .transforming(CodeTransform.ACCEPT_ALL, checkedAction)))); - var classTemplate = cf.build(ClassDesc.of("Test"), clb -> clb - .withMethodBody("test", MTD_void, ACC_STATIC, CodeBuilder::return_)); - // Indirect builders - assertThrows(IllegalArgumentException.class, () -> cf.transformClass(cf.parse(classTemplate), - ClassTransform.transformingMethodBodies(CodeTransform.endHandler(checkedAction) - .andThen(noopCodeTransform)))); + try { + TestUtil.runCodeHandler(checkedAction); + } catch (Throwable _) { + System.out.printf("Erroneous value %d%n", value); + } } static void check(boolean fails, Executable exec) { @@ -200,7 +250,10 @@ class InstructionValidationTest { IncrementInstruction.of(0, Short.MIN_VALUE); for (int i : new int[] {Short.MIN_VALUE - 1, Short.MAX_VALUE + 1}) { assertThrows(IllegalArgumentException.class, () -> IncrementInstruction.of(0, i)); - ensureFailFast(i, cob -> cob.iinc(0, i)); + TestUtil.runCodeHandler(cob -> { + assertThrows(IllegalArgumentException.class, () -> cob.iinc(0, i)); + cob.return_(); + }); } } @@ -215,5 +268,14 @@ class InstructionValidationTest { assertThrows(IllegalArgumentException.class, () -> NewMultiArrayInstruction.of(ce, -1)); assertThrows(IllegalArgumentException.class, () -> NewMultiArrayInstruction.of(ce, Integer.MIN_VALUE)); assertThrows(IllegalArgumentException.class, () -> NewMultiArrayInstruction.of(ce, Integer.MAX_VALUE)); + + TestUtil.runCodeHandler(cob -> { + assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, 0)); + assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, 0x100)); + assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, -1)); + assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, Integer.MIN_VALUE)); + assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, Integer.MAX_VALUE)); + cob.return_(); + }); } } diff --git a/test/jdk/jdk/classfile/TEST.properties b/test/jdk/jdk/classfile/TEST.properties index 2d8d20cf1f4..009b85888d1 100644 --- a/test/jdk/jdk/classfile/TEST.properties +++ b/test/jdk/jdk/classfile/TEST.properties @@ -2,4 +2,5 @@ maxOutputSize = 2500000 modules = \ java.base/jdk.internal.classfile.components \ java.base/jdk.internal.classfile.impl \ - java.base/jdk.internal.classfile.impl.verifier \ No newline at end of file + java.base/jdk.internal.classfile.impl.verifier +lib.dirs = helpers diff --git a/test/jdk/jdk/classfile/helpers/TestUtil.java b/test/jdk/jdk/classfile/helpers/TestUtil.java index 6baaea7800c..2ff4e5f275d 100644 --- a/test/jdk/jdk/classfile/helpers/TestUtil.java +++ b/test/jdk/jdk/classfile/helpers/TestUtil.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2022, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -24,29 +24,44 @@ package helpers; import jdk.internal.classfile.impl.LabelContext; import jdk.internal.classfile.impl.LabelImpl; + +import java.lang.classfile.ClassFile; +import java.lang.classfile.ClassTransform; +import java.lang.classfile.CodeBuilder; +import java.lang.classfile.CodeTransform; import java.lang.classfile.instruction.LocalVariable; import java.lang.classfile.instruction.LocalVariableType; -import java.io.FileOutputStream; +import java.lang.constant.ClassDesc; import java.util.Collection; +import java.util.function.Consumer; -public class TestUtil { +import static java.lang.classfile.ClassFile.ACC_STATIC; +import static java.lang.constant.ConstantDescs.MTD_void; + +public final class TestUtil { + + /// Run a code handler in different builders. + public static void runCodeHandler(Consumer handler) { + ClassFile cf = ClassFile.of(); + // Direct builders + cf.build(ClassDesc.of("Test"), clb -> clb.withMethodBody("test", MTD_void, ACC_STATIC, handler)); + // Chained builders + cf.build(ClassDesc.of("Test"), clb -> clb + .withMethodBody("test", MTD_void, ACC_STATIC, cob -> cob + .transforming(CodeTransform.ACCEPT_ALL, handler))); + // Indirect builders + var classTemplate = cf.build(ClassDesc.of("Test"), clb -> clb + .withMethodBody("test", MTD_void, ACC_STATIC, CodeBuilder::return_)); + cf.transformClass(cf.parse(classTemplate), + ClassTransform.transformingMethodBodies(CodeTransform.endHandler(handler) + .andThen(CodeBuilder::with))); + } public static void assertEmpty(Collection col) { if (!col.isEmpty()) throw new AssertionError(col); } - public static void writeClass(byte[] bytes, String fn) { - try { - FileOutputStream out = new FileOutputStream(fn); - out.write(bytes); - out.close(); - } catch (Exception ex) { - throw new InternalError(ex); - } - } - - public static class ExpectedLvRecord { int slot; String desc;