Skip to content

Commit 974f4da

Browse files
Jan Kratochvilgnu-andrew
Jan Kratochvil
authored and
gnu-andrew
committed
8352637: Enhance bytecode verification
Reviewed-by: fferrari, andrew Backport-of: 76bdbfbec7c7e8d25473109cb7c32a40a1a4108d
1 parent 25a9b9e commit 974f4da

File tree

5 files changed

+74
-31
lines changed

5 files changed

+74
-31
lines changed

‎src/hotspot/share/classfile/stackMapTable.cpp‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,16 @@ bool StackMapTable::match_stackmap(
122122
}
123123

124124
void StackMapTable::check_jump_target(
125-
StackMapFrame* frame, int32_t target, TRAPS) const {
125+
StackMapFrame* frame, int bci, int offset, TRAPS) const {
126126
ErrorContext ctx;
127+
// Jump targets must be within the method and the method size is limited. See JVMS 4.11
128+
int min_offset = -1 * max_method_code_size;
129+
if (offset < min_offset || offset > max_method_code_size) {
130+
frame->verifier()->verify_error(ErrorContext::bad_stackmap(bci, frame),
131+
"Illegal target of jump or branch (bci %d + offset %d)", bci, offset);
132+
return;
133+
}
134+
int target = bci + offset;
127135
bool match = match_stackmap(
128136
frame, target, true, false, &ctx, CHECK_VERIFY(frame->verifier()));
129137
if (!match || (target < 0 || target >= _code_length)) {

‎src/hotspot/share/classfile/stackMapTable.hpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class StackMapTable : public StackObj {
6969

7070
// Check jump instructions. Make sure there are no uninitialized
7171
// instances on backward branch.
72-
void check_jump_target(StackMapFrame* frame, int32_t target, TRAPS) const;
72+
void check_jump_target(StackMapFrame* frame, int bci, int offset, TRAPS) const;
7373

7474
// The following methods are only used inside this class.
7575

‎src/hotspot/share/classfile/verifier.cpp‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -717,7 +717,6 @@ void ClassVerifier::verify_method(const methodHandle& m, TRAPS) {
717717
// Merge with the next instruction
718718
{
719719
u2 index;
720-
int target;
721720
VerificationType type, type2;
722721
VerificationType atype;
723722

@@ -1534,9 +1533,8 @@ void ClassVerifier::verify_method(const methodHandle& m, TRAPS) {
15341533
case Bytecodes::_ifle:
15351534
current_frame.pop_stack(
15361535
VerificationType::integer_type(), CHECK_VERIFY(this));
1537-
target = bcs.dest();
15381536
stackmap_table.check_jump_target(
1539-
&current_frame, target, CHECK_VERIFY(this));
1537+
&current_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this));
15401538
no_control_flow = false; break;
15411539
case Bytecodes::_if_acmpeq :
15421540
case Bytecodes::_if_acmpne :
@@ -1547,19 +1545,16 @@ void ClassVerifier::verify_method(const methodHandle& m, TRAPS) {
15471545
case Bytecodes::_ifnonnull :
15481546
current_frame.pop_stack(
15491547
VerificationType::reference_check(), CHECK_VERIFY(this));
1550-
target = bcs.dest();
15511548
stackmap_table.check_jump_target
1552-
(&current_frame, target, CHECK_VERIFY(this));
1549+
(&current_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this));
15531550
no_control_flow = false; break;
15541551
case Bytecodes::_goto :
1555-
target = bcs.dest();
15561552
stackmap_table.check_jump_target(
1557-
&current_frame, target, CHECK_VERIFY(this));
1553+
&current_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this));
15581554
no_control_flow = true; break;
15591555
case Bytecodes::_goto_w :
1560-
target = bcs.dest_w();
15611556
stackmap_table.check_jump_target(
1562-
&current_frame, target, CHECK_VERIFY(this));
1557+
&current_frame, bcs.bci(), bcs.get_offset_s4(), CHECK_VERIFY(this));
15631558
no_control_flow = true; break;
15641559
case Bytecodes::_tableswitch :
15651560
case Bytecodes::_lookupswitch :
@@ -2208,15 +2203,14 @@ void ClassVerifier::verify_switch(
22082203
}
22092204
}
22102205
}
2211-
int target = bci + default_offset;
2212-
stackmap_table->check_jump_target(current_frame, target, CHECK_VERIFY(this));
2206+
stackmap_table->check_jump_target(current_frame, bci, default_offset, CHECK_VERIFY(this));
22132207
for (int i = 0; i < keys; i++) {
22142208
// Because check_jump_target() may safepoint, the bytecode could have
22152209
// moved, which means 'aligned_bcp' is no good and needs to be recalculated.
22162210
aligned_bcp = align_up(bcs->bcp() + 1, jintSize);
2217-
target = bci + (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
2211+
int offset = (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
22182212
stackmap_table->check_jump_target(
2219-
current_frame, target, CHECK_VERIFY(this));
2213+
current_frame, bci, offset, CHECK_VERIFY(this));
22202214
}
22212215
NOT_PRODUCT(aligned_bcp = NULL); // no longer valid at this point
22222216
}
@@ -2479,8 +2473,13 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) {
24792473
break;
24802474

24812475
case Bytecodes::_goto:
2482-
case Bytecodes::_goto_w:
2483-
target = (opcode == Bytecodes::_goto ? bcs.dest() : bcs.dest_w());
2476+
case Bytecodes::_goto_w: {
2477+
int offset = (opcode == Bytecodes::_goto ? bcs.get_offset_s2() : bcs.get_offset_s4());
2478+
int min_offset = -1 * max_method_code_size;
2479+
// Check offset for overflow
2480+
if (offset < min_offset || offset > max_method_code_size) return false;
2481+
2482+
target = bci + offset;
24842483
if (visited_branches->contains(bci)) {
24852484
if (bci_stack->is_empty()) {
24862485
if (handler_stack->is_empty()) {
@@ -2501,6 +2500,7 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) {
25012500
visited_branches->append(bci);
25022501
}
25032502
break;
2503+
}
25042504

25052505
// Check that all switch alternatives end in 'athrow' bytecodes. Since it
25062506
// is difficult to determine where each switch alternative ends, parse
@@ -2537,7 +2537,10 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) {
25372537

25382538
// Push the switch alternatives onto the stack.
25392539
for (int i = 0; i < keys; i++) {
2540-
u4 target = bci + (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
2540+
int min_offset = -1 * max_method_code_size;
2541+
int offset = (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
2542+
if (offset < min_offset || offset > max_method_code_size) return false;
2543+
u4 target = bci + offset;
25412544
if (target > code_length) return false;
25422545
bci_stack->push(target);
25432546
}

‎src/hotspot/share/interpreter/bytecodeStream.hpp‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,23 @@ class BaseBytecodeStream: StackObj {
100100
void set_next_bci(int bci) { assert(0 <= bci && bci <= method()->code_size(), "illegal bci"); _next_bci = bci; }
101101

102102
// Bytecode-specific attributes
103-
int dest() const { return bci() + bytecode().get_offset_s2(raw_code()); }
104-
int dest_w() const { return bci() + bytecode().get_offset_s4(raw_code()); }
103+
int get_offset_s2() const { return bytecode().get_offset_s2(raw_code()); }
104+
int get_offset_s4() const { return bytecode().get_offset_s4(raw_code()); }
105+
106+
// These methods are not safe to use before or during verification as they may
107+
// have large offsets and cause overflows
108+
int dest() const {
109+
int min_offset = -1 * max_method_code_size;
110+
int offset = bytecode().get_offset_s2(raw_code());
111+
guarantee(offset >= min_offset && offset <= max_method_code_size, "must be");
112+
return bci() + offset;
113+
}
114+
int dest_w() const {
115+
int min_offset = -1 * max_method_code_size;
116+
int offset = bytecode().get_offset_s4(raw_code());
117+
guarantee(offset >= min_offset && offset <= max_method_code_size, "must be");
118+
return bci() + offset;
119+
}
105120

106121
// One-byte indices.
107122
int get_index_u1() const { assert_raw_index_size(1); return *(jubyte*)(bcp()+1); }

‎src/java.base/share/native/libverify/check_code.c‎

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,8 @@ static jboolean is_superclass(context_type *, fullinfo_type);
398398

399399
static void initialize_exception_table(context_type *);
400400
static int instruction_length(unsigned char *iptr, unsigned char *end);
401-
static jboolean isLegalTarget(context_type *, int offset);
401+
static jboolean isLegalOffset(context_type *, int bci, int offset);
402+
static jboolean isLegalTarget(context_type *, int target);
402403
static void verify_constant_pool_type(context_type *, int, unsigned);
403404

404405
static void initialize_dataflow(context_type *);
@@ -1169,9 +1170,9 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset)
11691170
case JVM_OPC_goto: {
11701171
/* Set the ->operand to be the instruction number of the target. */
11711172
int jump = (((signed char)(code[offset+1])) << 8) + code[offset+2];
1172-
int target = offset + jump;
1173-
if (!isLegalTarget(context, target))
1173+
if (!isLegalOffset(context, offset, jump))
11741174
CCerror(context, "Illegal target of jump or branch");
1175+
int target = offset + jump;
11751176
this_idata->operand.i = code_data[target];
11761177
break;
11771178
}
@@ -1185,9 +1186,9 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset)
11851186
int jump = (((signed char)(code[offset+1])) << 24) +
11861187
(code[offset+2] << 16) + (code[offset+3] << 8) +
11871188
(code[offset + 4]);
1188-
int target = offset + jump;
1189-
if (!isLegalTarget(context, target))
1189+
if (!isLegalOffset(context, offset, jump))
11901190
CCerror(context, "Illegal target of jump or branch");
1191+
int target = offset + jump;
11911192
this_idata->operand.i = code_data[target];
11921193
break;
11931194
}
@@ -1226,13 +1227,16 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset)
12261227
}
12271228
}
12281229
saved_operand = NEW(int, keys + 2);
1229-
if (!isLegalTarget(context, offset + _ck_ntohl(lpc[0])))
1230+
int jump = _ck_ntohl(lpc[0]);
1231+
if (!isLegalOffset(context, offset, jump))
12301232
CCerror(context, "Illegal default target in switch");
1231-
saved_operand[keys + 1] = code_data[offset + _ck_ntohl(lpc[0])];
1233+
int target = offset + jump;
1234+
saved_operand[keys + 1] = code_data[target];
12321235
for (k = keys, lptr = &lpc[3]; --k >= 0; lptr += delta) {
1233-
int target = offset + _ck_ntohl(lptr[0]);
1234-
if (!isLegalTarget(context, target))
1236+
jump = _ck_ntohl(lptr[0]);
1237+
if (!isLegalOffset(context, offset, jump))
12351238
CCerror(context, "Illegal branch in tableswitch");
1239+
target = offset + jump;
12361240
saved_operand[k + 1] = code_data[target];
12371241
}
12381242
saved_operand[0] = keys + 1; /* number of successors */
@@ -1756,11 +1760,24 @@ static int instruction_length(unsigned char *iptr, unsigned char *end)
17561760

17571761
/* Given the target of a branch, make sure that it's a legal target. */
17581762
static jboolean
1759-
isLegalTarget(context_type *context, int offset)
1763+
isLegalTarget(context_type *context, int target)
1764+
{
1765+
int code_length = context->code_length;
1766+
int *code_data = context->code_data;
1767+
return (target >= 0 && target < code_length && code_data[target] >= 0);
1768+
}
1769+
1770+
/* Given a bci and offset, make sure the offset is valid and the target is legal */
1771+
static jboolean
1772+
isLegalOffset(context_type *context, int bci, int offset)
17601773
{
17611774
int code_length = context->code_length;
17621775
int *code_data = context->code_data;
1763-
return (offset >= 0 && offset < code_length && code_data[offset] >= 0);
1776+
int max_offset = 65535; // JVMS 4.11
1777+
int min_offset = -65535;
1778+
if (offset < min_offset || offset > max_offset) return JNI_FALSE;
1779+
int target = bci + offset;
1780+
return (target >= 0 && target < code_length && code_data[target] >= 0);
17641781
}
17651782

17661783

0 commit comments

Comments
 (0)