Skip to content

Commit 05d9cf5

Browse files
Jan KratochvilRealCLanger
Jan Kratochvil
authored and
RealCLanger
committed
8352637: Enhance bytecode verification
Reviewed-by: fferrari Backport-of: d9bf0c2ca2d52d783a8122504cac9566d42b22df
1 parent ba6bbe5 commit 05d9cf5

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
@@ -792,7 +792,6 @@ void ClassVerifier::verify_method(const methodHandle& m, TRAPS) {
792792
// Merge with the next instruction
793793
{
794794
u2 index;
795-
int target;
796795
VerificationType type, type2;
797796
VerificationType atype;
798797

@@ -1608,9 +1607,8 @@ void ClassVerifier::verify_method(const methodHandle& m, TRAPS) {
16081607
case Bytecodes::_ifle:
16091608
current_frame.pop_stack(
16101609
VerificationType::integer_type(), CHECK_VERIFY(this));
1611-
target = bcs.dest();
16121610
stackmap_table.check_jump_target(
1613-
&current_frame, target, CHECK_VERIFY(this));
1611+
&current_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this));
16141612
no_control_flow = false; break;
16151613
case Bytecodes::_if_acmpeq :
16161614
case Bytecodes::_if_acmpne :
@@ -1621,19 +1619,16 @@ void ClassVerifier::verify_method(const methodHandle& m, TRAPS) {
16211619
case Bytecodes::_ifnonnull :
16221620
current_frame.pop_stack(
16231621
VerificationType::reference_check(), CHECK_VERIFY(this));
1624-
target = bcs.dest();
16251622
stackmap_table.check_jump_target
1626-
(&current_frame, target, CHECK_VERIFY(this));
1623+
(&current_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this));
16271624
no_control_flow = false; break;
16281625
case Bytecodes::_goto :
1629-
target = bcs.dest();
16301626
stackmap_table.check_jump_target(
1631-
&current_frame, target, CHECK_VERIFY(this));
1627+
&current_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this));
16321628
no_control_flow = true; break;
16331629
case Bytecodes::_goto_w :
1634-
target = bcs.dest_w();
16351630
stackmap_table.check_jump_target(
1636-
&current_frame, target, CHECK_VERIFY(this));
1631+
&current_frame, bcs.bci(), bcs.get_offset_s4(), CHECK_VERIFY(this));
16371632
no_control_flow = true; break;
16381633
case Bytecodes::_tableswitch :
16391634
case Bytecodes::_lookupswitch :
@@ -2283,15 +2278,14 @@ void ClassVerifier::verify_switch(
22832278
}
22842279
}
22852280
}
2286-
int target = bci + default_offset;
2287-
stackmap_table->check_jump_target(current_frame, target, CHECK_VERIFY(this));
2281+
stackmap_table->check_jump_target(current_frame, bci, default_offset, CHECK_VERIFY(this));
22882282
for (int i = 0; i < keys; i++) {
22892283
// Because check_jump_target() may safepoint, the bytecode could have
22902284
// moved, which means 'aligned_bcp' is no good and needs to be recalculated.
22912285
aligned_bcp = align_up(bcs->bcp() + 1, jintSize);
2292-
target = bci + (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
2286+
int offset = (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
22932287
stackmap_table->check_jump_target(
2294-
current_frame, target, CHECK_VERIFY(this));
2288+
current_frame, bci, offset, CHECK_VERIFY(this));
22952289
}
22962290
NOT_PRODUCT(aligned_bcp = NULL); // no longer valid at this point
22972291
}
@@ -2550,8 +2544,13 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) {
25502544
break;
25512545

25522546
case Bytecodes::_goto:
2553-
case Bytecodes::_goto_w:
2554-
target = (opcode == Bytecodes::_goto ? bcs.dest() : bcs.dest_w());
2547+
case Bytecodes::_goto_w: {
2548+
int offset = (opcode == Bytecodes::_goto ? bcs.get_offset_s2() : bcs.get_offset_s4());
2549+
int min_offset = -1 * max_method_code_size;
2550+
// Check offset for overflow
2551+
if (offset < min_offset || offset > max_method_code_size) return false;
2552+
2553+
target = bci + offset;
25552554
if (visited_branches->contains(bci)) {
25562555
if (bci_stack->is_empty()) {
25572556
if (handler_stack->is_empty()) {
@@ -2572,6 +2571,7 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) {
25722571
visited_branches->append(bci);
25732572
}
25742573
break;
2574+
}
25752575

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

26092609
// Push the switch alternatives onto the stack.
26102610
for (int i = 0; i < keys; i++) {
2611-
u4 target = bci + (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
2611+
int min_offset = -1 * max_method_code_size;
2612+
int offset = (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize);
2613+
if (offset < min_offset || offset > max_method_code_size) return false;
2614+
u4 target = bci + offset;
26122615
if (target > code_length) return false;
26132616
bci_stack->push(target);
26142617
}

‎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
@@ -395,7 +395,8 @@ static jboolean is_superclass(context_type *, fullinfo_type);
395395

396396
static void initialize_exception_table(context_type *);
397397
static int instruction_length(unsigned char *iptr, unsigned char *end);
398-
static jboolean isLegalTarget(context_type *, int offset);
398+
static jboolean isLegalOffset(context_type *, int bci, int offset);
399+
static jboolean isLegalTarget(context_type *, int target);
399400
static void verify_constant_pool_type(context_type *, int, unsigned);
400401

401402
static void initialize_dataflow(context_type *);
@@ -1154,9 +1155,9 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset)
11541155
case JVM_OPC_goto: {
11551156
/* Set the ->operand to be the instruction number of the target. */
11561157
int jump = (((signed char)(code[offset+1])) << 8) + code[offset+2];
1157-
int target = offset + jump;
1158-
if (!isLegalTarget(context, target))
1158+
if (!isLegalOffset(context, offset, jump))
11591159
CCerror(context, "Illegal target of jump or branch");
1160+
int target = offset + jump;
11601161
this_idata->operand.i = code_data[target];
11611162
break;
11621163
}
@@ -1170,9 +1171,9 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset)
11701171
int jump = (((signed char)(code[offset+1])) << 24) +
11711172
(code[offset+2] << 16) + (code[offset+3] << 8) +
11721173
(code[offset + 4]);
1173-
int target = offset + jump;
1174-
if (!isLegalTarget(context, target))
1174+
if (!isLegalOffset(context, offset, jump))
11751175
CCerror(context, "Illegal target of jump or branch");
1176+
int target = offset + jump;
11761177
this_idata->operand.i = code_data[target];
11771178
break;
11781179
}
@@ -1211,13 +1212,16 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset)
12111212
}
12121213
}
12131214
saved_operand = NEW(int, keys + 2);
1214-
if (!isLegalTarget(context, offset + _ck_ntohl(lpc[0])))
1215+
int jump = _ck_ntohl(lpc[0]);
1216+
if (!isLegalOffset(context, offset, jump))
12151217
CCerror(context, "Illegal default target in switch");
1216-
saved_operand[keys + 1] = code_data[offset + _ck_ntohl(lpc[0])];
1218+
int target = offset + jump;
1219+
saved_operand[keys + 1] = code_data[target];
12171220
for (k = keys, lptr = &lpc[3]; --k >= 0; lptr += delta) {
1218-
int target = offset + _ck_ntohl(lptr[0]);
1219-
if (!isLegalTarget(context, target))
1221+
jump = _ck_ntohl(lptr[0]);
1222+
if (!isLegalOffset(context, offset, jump))
12201223
CCerror(context, "Illegal branch in tableswitch");
1224+
target = offset + jump;
12211225
saved_operand[k + 1] = code_data[target];
12221226
}
12231227
saved_operand[0] = keys + 1; /* number of successors */
@@ -1746,11 +1750,24 @@ static int instruction_length(unsigned char *iptr, unsigned char *end)
17461750

17471751
/* Given the target of a branch, make sure that it's a legal target. */
17481752
static jboolean
1749-
isLegalTarget(context_type *context, int offset)
1753+
isLegalTarget(context_type *context, int target)
1754+
{
1755+
int code_length = context->code_length;
1756+
int *code_data = context->code_data;
1757+
return (target >= 0 && target < code_length && code_data[target] >= 0);
1758+
}
1759+
1760+
/* Given a bci and offset, make sure the offset is valid and the target is legal */
1761+
static jboolean
1762+
isLegalOffset(context_type *context, int bci, int offset)
17501763
{
17511764
int code_length = context->code_length;
17521765
int *code_data = context->code_data;
1753-
return (offset >= 0 && offset < code_length && code_data[offset] >= 0);
1766+
int max_offset = 65535; // JVMS 4.11
1767+
int min_offset = -65535;
1768+
if (offset < min_offset || offset > max_offset) return JNI_FALSE;
1769+
int target = bci + offset;
1770+
return (target >= 0 && target < code_length && code_data[target] >= 0);
17541771
}
17551772

17561773

0 commit comments

Comments
 (0)