Skip to content

Commit 3f59eae

Browse files
liachliach
committed
8361102: java.lang.classfile.CodeBuilder.branch(Opcode op, Label target) doesn't throw IllegalArgumentException - if op is not of Opcode.Kind.BRANCH
Reviewed-by: asotona
1 parent 46988e1 commit 3f59eae

File tree

5 files changed

+145
-52
lines changed

5 files changed

+145
-52
lines changed

‎src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,7 @@ public void writeIndex(PoolEntry entry) {
399399
writeU2(cpIndex(entry));
400400
}
401401

402+
// Null checks entry
402403
public void writeIndex(int bytecode, PoolEntry entry) {
403404
writeU1U2(bytecode, cpIndex(entry));
404405
}

‎src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java‎

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -546,8 +546,8 @@ private void writeLongLabelOffset(int instructionPc, Label label) {
546546
}
547547

548548
private void writeShortJump(int bytecode, Label target) {
549+
int targetBci = labelToBci(target); // implicit null check
549550
int instructionPc = curPc();
550-
int targetBci = labelToBci(target);
551551

552552
// algebraic union of jump | (instructionPc, target), distinguished by null == target.
553553
int jumpOrInstructionPc;
@@ -570,6 +570,7 @@ private void writeShortJump(int bytecode, Label target) {
570570
}
571571

572572
private void writeLongJump(int bytecode, Label target) {
573+
Objects.requireNonNull(target); // before any write
573574
int instructionPc = curPc();
574575
bytecodesBufWriter.writeU1(bytecode);
575576
writeLongLabelOffset(instructionPc, target);
@@ -617,20 +618,24 @@ private void writeParsedLongLabel(int jumpOrInstructionPc, Label nullOrTarget) {
617618
}
618619

619620
public void writeLookupSwitch(Label defaultTarget, List<SwitchCase> cases) {
621+
cases = new ArrayList<>(cases); // cases may be untrusted
622+
for (var each : cases) {
623+
Objects.requireNonNull(each); // single null case may exist
624+
}
625+
cases.sort(new Comparator<>() {
626+
@Override
627+
public int compare(SwitchCase c1, SwitchCase c2) {
628+
return Integer.compare(c1.caseValue(), c2.caseValue());
629+
}
630+
});
631+
// validation end
620632
int instructionPc = curPc();
621633
bytecodesBufWriter.writeU1(LOOKUPSWITCH);
622634
int pad = 4 - (curPc() % 4);
623635
if (pad != 4)
624636
bytecodesBufWriter.skip(pad); // padding content can be anything
625637
writeLongLabelOffset(instructionPc, defaultTarget);
626638
bytecodesBufWriter.writeInt(cases.size());
627-
cases = new ArrayList<>(cases);
628-
cases.sort(new Comparator<>() {
629-
@Override
630-
public int compare(SwitchCase c1, SwitchCase c2) {
631-
return Integer.compare(c1.caseValue(), c2.caseValue());
632-
}
633-
});
634639
for (var c : cases) {
635640
bytecodesBufWriter.writeInt(c.caseValue());
636641
var target = c.target();
@@ -639,17 +644,18 @@ public int compare(SwitchCase c1, SwitchCase c2) {
639644
}
640645

641646
public void writeTableSwitch(int low, int high, Label defaultTarget, List<SwitchCase> cases) {
647+
var caseMap = new HashMap<Integer, Label>(cases.size()); // cases may be untrusted
648+
for (var c : cases) {
649+
caseMap.put(c.caseValue(), c.target());
650+
}
651+
// validation end
642652
int instructionPc = curPc();
643653
bytecodesBufWriter.writeU1(TABLESWITCH);
644654
int pad = 4 - (curPc() % 4);
645655
if (pad != 4)
646656
bytecodesBufWriter.skip(pad); // padding content can be anything
647657
writeLongLabelOffset(instructionPc, defaultTarget);
648658
bytecodesBufWriter.writeIntInt(low, high);
649-
var caseMap = new HashMap<Integer, Label>(cases.size());
650-
for (var c : cases) {
651-
caseMap.put(c.caseValue(), c.target());
652-
}
653659
for (long l = low; l<=high; l++) {
654660
var target = caseMap.getOrDefault((int)l, defaultTarget);
655661
writeLongLabelOffset(instructionPc, target);
@@ -928,6 +934,7 @@ public CodeBuilder invoke(Opcode opcode, MemberRefEntry ref) {
928934
int slots = Util.parameterSlots(Util.methodTypeSymbol(ref.type())) + 1;
929935
writeInvokeInterface(opcode, (InterfaceMethodRefEntry) ref, slots);
930936
} else {
937+
Util.checkKind(opcode, Opcode.Kind.INVOKE);
931938
writeInvokeNormal(opcode, ref);
932939
}
933940
return this;
@@ -959,7 +966,8 @@ public CodeBuilder getfield(ClassDesc owner, String name, ClassDesc type) {
959966

960967
@Override
961968
public CodeBuilder fieldAccess(Opcode opcode, FieldRefEntry ref) {
962-
bytecodesBufWriter.writeIndex(opcode.bytecode(), ref);
969+
Util.checkKind(opcode, Opcode.Kind.FIELD_ACCESS);
970+
writeFieldAccess(opcode, ref);
963971
return this;
964972
}
965973

@@ -977,6 +985,7 @@ public CodeBuilder arrayStore(TypeKind tk) {
977985

978986
@Override
979987
public CodeBuilder branch(Opcode op, Label target) {
988+
Util.checkKind(op, Opcode.Kind.BRANCH);
980989
writeBranch(op, target);
981990
return this;
982991
}
@@ -1640,6 +1649,8 @@ public CodeBuilder ixor() {
16401649

16411650
@Override
16421651
public CodeBuilder lookupswitch(Label defaultTarget, List<SwitchCase> cases) {
1652+
Objects.requireNonNull(defaultTarget);
1653+
// check cases when we sort them
16431654
writeLookupSwitch(defaultTarget, cases);
16441655
return this;
16451656
}
@@ -1799,6 +1810,7 @@ public CodeBuilder monitorexit() {
17991810

18001811
@Override
18011812
public CodeBuilder multianewarray(ClassEntry array, int dims) {
1813+
BytecodeHelpers.validateMultiArrayDimensions(dims);
18021814
writeNewMultidimensionalArray(dims, array);
18031815
return this;
18041816
}
@@ -1845,6 +1857,8 @@ public CodeBuilder swap() {
18451857

18461858
@Override
18471859
public CodeBuilder tableswitch(int low, int high, Label defaultTarget, List<SwitchCase> cases) {
1860+
Objects.requireNonNull(defaultTarget);
1861+
// check cases when we write them
18481862
writeTableSwitch(low, high, defaultTarget, cases);
18491863
return this;
18501864
}

‎test/jdk/jdk/classfile/InstructionValidationTest.java‎

Lines changed: 85 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
/*
2525
* @test
26-
* @bug 8341277
26+
* @bug 8341277 8361102
2727
* @summary Testing ClassFile instruction argument validation.
2828
* @run junit InstructionValidationTest
2929
*/
@@ -32,24 +32,77 @@
3232
import java.lang.classfile.constantpool.ClassEntry;
3333
import java.lang.classfile.constantpool.ConstantPoolBuilder;
3434
import java.lang.classfile.instruction.*;
35-
import java.lang.constant.ClassDesc;
36-
import java.lang.reflect.Parameter;
35+
import java.util.Collections;
3736
import java.util.List;
3837
import java.util.function.Consumer;
3938
import java.util.function.ObjIntConsumer;
4039
import java.util.stream.Stream;
4140

41+
import helpers.TestUtil;
4242
import org.junit.jupiter.api.Test;
4343
import org.junit.jupiter.api.function.Executable;
4444

45-
import static java.lang.classfile.ClassFile.ACC_STATIC;
4645
import static java.lang.constant.ConstantDescs.*;
46+
import static helpers.TestConstants.*;
4747
import static org.junit.jupiter.api.Assertions.*;
4848
import static java.lang.classfile.Opcode.*;
4949
import static org.junit.jupiter.api.Assertions.assertThrows;
5050

5151
class InstructionValidationTest {
5252

53+
@Test
54+
void testOpcodeInCodeBuilder() {
55+
TestUtil.runCodeHandler(cob -> {
56+
var mref = cob.constantPool().methodRefEntry(CD_System, "exit", MTD_INT_VOID);
57+
var fref = cob.constantPool().fieldRefEntry(CD_System, "out", CD_PrintStream);
58+
var label = cob.newLabel();
59+
60+
// Sanity
61+
cob.iconst_0();
62+
assertDoesNotThrow(() -> cob.invoke(INVOKESTATIC, mref));
63+
assertDoesNotThrow(() -> cob.fieldAccess(GETSTATIC, fref));
64+
cob.pop();
65+
assertDoesNotThrow(() -> cob.branch(GOTO, label));
66+
67+
// Opcode NPE
68+
assertThrows(NullPointerException.class, () -> cob.invoke(null, mref));
69+
assertThrows(NullPointerException.class, () -> cob.fieldAccess(null, fref));
70+
assertThrows(NullPointerException.class, () -> cob.branch(null, label));
71+
72+
// Opcode IAE
73+
assertThrows(IllegalArgumentException.class, () -> cob.invoke(IFNE, mref));
74+
assertThrows(IllegalArgumentException.class, () -> cob.fieldAccess(JSR, fref));
75+
assertThrows(IllegalArgumentException.class, () -> cob.branch(CHECKCAST, label));
76+
77+
// Wrap up
78+
cob.labelBinding(label);
79+
cob.return_();
80+
});
81+
}
82+
83+
@Test
84+
void testLongJump() {
85+
TestUtil.runCodeHandler(cob -> {
86+
assertThrows(NullPointerException.class, () -> cob.goto_w(null));
87+
// Ensures nothing redundant is written in case of failure
88+
cob.return_();
89+
});
90+
}
91+
92+
@Test
93+
void testSwitch() {
94+
TestUtil.runCodeHandler(cob -> {
95+
assertThrows(NullPointerException.class, () -> cob.tableswitch(-1, 1, cob.startLabel(), null));
96+
assertThrows(NullPointerException.class, () -> cob.lookupswitch(cob.startLabel(), null));
97+
assertThrows(NullPointerException.class, () -> cob.tableswitch(-1, 1, cob.startLabel(), Collections.singletonList(null)));
98+
assertThrows(NullPointerException.class, () -> cob.lookupswitch(cob.startLabel(), Collections.singletonList(null)));
99+
assertThrows(NullPointerException.class, () -> cob.tableswitch(-1, 1, null, List.of()));
100+
assertThrows(NullPointerException.class, () -> cob.lookupswitch(null, List.of()));
101+
// Ensures nothing redundant is written in case of failure
102+
cob.return_();
103+
});
104+
}
105+
53106
@Test
54107
void testArgumentConstant() {
55108
assertDoesNotThrow(() -> ConstantInstruction.ofArgument(SIPUSH, 0));
@@ -63,6 +116,14 @@ void testArgumentConstant() {
63116
assertThrows(IllegalArgumentException.class, () -> ConstantInstruction.ofArgument(SIPUSH, (int) Short.MAX_VALUE + 1));
64117
assertThrows(IllegalArgumentException.class, () -> ConstantInstruction.ofArgument(BIPUSH, (int) Byte.MIN_VALUE - 1));
65118
assertThrows(IllegalArgumentException.class, () -> ConstantInstruction.ofArgument(BIPUSH, (int) Byte.MAX_VALUE + 1));
119+
120+
TestUtil.runCodeHandler(cob -> {
121+
assertThrows(IllegalArgumentException.class, () -> cob.sipush((int) Short.MIN_VALUE - 1));
122+
assertThrows(IllegalArgumentException.class, () -> cob.sipush((int) Short.MAX_VALUE + 1));
123+
assertThrows(IllegalArgumentException.class, () -> cob.bipush((int) Byte.MIN_VALUE - 1));
124+
assertThrows(IllegalArgumentException.class, () -> cob.bipush((int) Byte.MAX_VALUE + 1));
125+
cob.return_();
126+
});
66127
}
67128

68129
/**
@@ -163,26 +224,15 @@ record Result(boolean shouldFail, int slot) {
163224
// CodeBuilder can fail with IAE due to other reasons, so we cannot check
164225
// "success" but can ensure things fail fast
165226
static void ensureFailFast(int value, Consumer<CodeBuilder> action) {
166-
// Can fail with AssertionError instead of IAE
167227
Consumer<CodeBuilder> checkedAction = cob -> {
168-
action.accept(cob);
169-
fail("Bad slot access did not fail fast: " + value);
228+
assertThrows(IllegalArgumentException.class, () -> action.accept(cob));
229+
cob.return_();
170230
};
171-
var cf = ClassFile.of();
172-
CodeTransform noopCodeTransform = CodeBuilder::with;
173-
// Direct builders
174-
assertThrows(IllegalArgumentException.class, () -> cf.build(ClassDesc.of("Test"), clb -> clb
175-
.withMethodBody("test", MTD_void, ACC_STATIC, checkedAction)));
176-
// Chained builders
177-
assertThrows(IllegalArgumentException.class, () -> cf.build(ClassDesc.of("Test"), clb -> clb
178-
.withMethodBody("test", MTD_void, ACC_STATIC, cob -> cob
179-
.transforming(CodeTransform.ACCEPT_ALL, checkedAction))));
180-
var classTemplate = cf.build(ClassDesc.of("Test"), clb -> clb
181-
.withMethodBody("test", MTD_void, ACC_STATIC, CodeBuilder::return_));
182-
// Indirect builders
183-
assertThrows(IllegalArgumentException.class, () -> cf.transformClass(cf.parse(classTemplate),
184-
ClassTransform.transformingMethodBodies(CodeTransform.endHandler(checkedAction)
185-
.andThen(noopCodeTransform))));
231+
try {
232+
TestUtil.runCodeHandler(checkedAction);
233+
} catch (Throwable _) {
234+
System.out.printf("Erroneous value %d%n", value);
235+
}
186236
}
187237

188238
static void check(boolean fails, Executable exec) {
@@ -200,7 +250,10 @@ void testIincConstant() {
200250
IncrementInstruction.of(0, Short.MIN_VALUE);
201251
for (int i : new int[] {Short.MIN_VALUE - 1, Short.MAX_VALUE + 1}) {
202252
assertThrows(IllegalArgumentException.class, () -> IncrementInstruction.of(0, i));
203-
ensureFailFast(i, cob -> cob.iinc(0, i));
253+
TestUtil.runCodeHandler(cob -> {
254+
assertThrows(IllegalArgumentException.class, () -> cob.iinc(0, i));
255+
cob.return_();
256+
});
204257
}
205258
}
206259

@@ -215,5 +268,14 @@ void testNewMultiArrayDimension() {
215268
assertThrows(IllegalArgumentException.class, () -> NewMultiArrayInstruction.of(ce, -1));
216269
assertThrows(IllegalArgumentException.class, () -> NewMultiArrayInstruction.of(ce, Integer.MIN_VALUE));
217270
assertThrows(IllegalArgumentException.class, () -> NewMultiArrayInstruction.of(ce, Integer.MAX_VALUE));
271+
272+
TestUtil.runCodeHandler(cob -> {
273+
assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, 0));
274+
assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, 0x100));
275+
assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, -1));
276+
assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, Integer.MIN_VALUE));
277+
assertThrows(IllegalArgumentException.class, () -> cob.multianewarray(ce, Integer.MAX_VALUE));
278+
cob.return_();
279+
});
218280
}
219281
}

‎test/jdk/jdk/classfile/TEST.properties‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@ maxOutputSize = 2500000
22
modules = \
33
java.base/jdk.internal.classfile.components \
44
java.base/jdk.internal.classfile.impl \
5-
java.base/jdk.internal.classfile.impl.verifier
5+
java.base/jdk.internal.classfile.impl.verifier
6+
lib.dirs = helpers

‎test/jdk/jdk/classfile/helpers/TestUtil.java‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2022, 2025, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -24,29 +24,44 @@
2424

2525
import jdk.internal.classfile.impl.LabelContext;
2626
import jdk.internal.classfile.impl.LabelImpl;
27+
28+
import java.lang.classfile.ClassFile;
29+
import java.lang.classfile.ClassTransform;
30+
import java.lang.classfile.CodeBuilder;
31+
import java.lang.classfile.CodeTransform;
2732
import java.lang.classfile.instruction.LocalVariable;
2833
import java.lang.classfile.instruction.LocalVariableType;
2934

30-
import java.io.FileOutputStream;
35+
import java.lang.constant.ClassDesc;
3136
import java.util.Collection;
32-
33-
public class TestUtil {
37+
import java.util.function.Consumer;
38+
39+
import static java.lang.classfile.ClassFile.ACC_STATIC;
40+
import static java.lang.constant.ConstantDescs.MTD_void;
41+
42+
public final class TestUtil {
43+
44+
/// Run a code handler in different builders.
45+
public static void runCodeHandler(Consumer<CodeBuilder> handler) {
46+
ClassFile cf = ClassFile.of();
47+
// Direct builders
48+
cf.build(ClassDesc.of("Test"), clb -> clb.withMethodBody("test", MTD_void, ACC_STATIC, handler));
49+
// Chained builders
50+
cf.build(ClassDesc.of("Test"), clb -> clb
51+
.withMethodBody("test", MTD_void, ACC_STATIC, cob -> cob
52+
.transforming(CodeTransform.ACCEPT_ALL, handler)));
53+
// Indirect builders
54+
var classTemplate = cf.build(ClassDesc.of("Test"), clb -> clb
55+
.withMethodBody("test", MTD_void, ACC_STATIC, CodeBuilder::return_));
56+
cf.transformClass(cf.parse(classTemplate),
57+
ClassTransform.transformingMethodBodies(CodeTransform.endHandler(handler)
58+
.andThen(CodeBuilder::with)));
59+
}
3460

3561
public static void assertEmpty(Collection<?> col) {
3662
if (!col.isEmpty()) throw new AssertionError(col);
3763
}
3864

39-
public static void writeClass(byte[] bytes, String fn) {
40-
try {
41-
FileOutputStream out = new FileOutputStream(fn);
42-
out.write(bytes);
43-
out.close();
44-
} catch (Exception ex) {
45-
throw new InternalError(ex);
46-
}
47-
}
48-
49-
5065
public static class ExpectedLvRecord {
5166
int slot;
5267
String desc;

0 commit comments

Comments
 (0)