8336042: Caller/callee param size mismatch in deoptimization causes crash

Co-authored-by: Richard Reingruber <[email protected]>
Reviewed-by: pchilanomate, rrich, vlivanov, never

Author: dean-long

src/hotspot/cpu/aarch64/abstractInterpreter_aarch64.cpp

@@ -149,7 +149,8 @@ void AbstractInterpreter::layout_activation(Method* method,
 
 #ifdef ASSERT
   if (caller->is_interpreted_frame()) {
-    assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
+    assert(locals <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(locals >= interpreter_frame->sender_sp() + max_locals - 1, "bad placement");
   }
 #endif
 

src/hotspot/cpu/arm/abstractInterpreter_arm.cpp

@@ -131,6 +131,15 @@ void AbstractInterpreter::layout_activation(Method* method,
 
   intptr_t* locals = interpreter_frame->sender_sp() + max_locals - 1;
 
+#ifdef ASSERT
+  if (caller->is_interpreted_frame()) {
+    // Test exact placement on top of caller args
+    intptr_t* l2 = caller->interpreter_frame_last_sp() + caller_actual_parameters - 1;
+    assert(l2 <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(l2 >= locals, "bad placement");
+  }
+#endif
+
   interpreter_frame->interpreter_frame_set_locals(locals);
   BasicObjectLock* montop = interpreter_frame->interpreter_frame_monitor_begin();
   BasicObjectLock* monbot = montop - moncount;

src/hotspot/cpu/ppc/abstractInterpreter_ppc.cpp

@@ -128,6 +128,15 @@ void AbstractInterpreter::layout_activation(Method* method,
     caller->interpreter_frame_esp() + caller_actual_parameters :
     caller->sp() + method->max_locals() - 1 + (frame::java_abi_size / Interpreter::stackElementSize);
 
+#ifdef ASSERT
+  if (caller->is_interpreted_frame()) {
+    assert(locals_base <= caller->interpreter_frame_expression_stack(), "bad placement");
+    const int caller_abi_bytesize = (is_bottom_frame ? frame::top_ijava_frame_abi_size : frame::parent_ijava_frame_abi_size);
+    intptr_t* l2 = caller->sp() + method->max_locals() - 1 + (caller_abi_bytesize / Interpreter::stackElementSize);
+    assert(locals_base >= l2, "bad placement");
+  }
+#endif
+
   intptr_t* monitor_base = caller->sp() - frame::ijava_state_size / Interpreter::stackElementSize;
   intptr_t* monitor      = monitor_base - (moncount * frame::interpreter_frame_monitor_size());
   intptr_t* esp_base     = monitor - 1;

src/hotspot/cpu/riscv/abstractInterpreter_riscv.cpp

@@ -141,7 +141,8 @@ void AbstractInterpreter::layout_activation(Method* method,
 
 #ifdef ASSERT
   if (caller->is_interpreted_frame()) {
-    assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
+    assert(locals <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(locals >= interpreter_frame->sender_sp() + max_locals - 1, "bad placement");
   }
 #endif
 

src/hotspot/cpu/s390/abstractInterpreter_s390.cpp

@@ -182,6 +182,13 @@ void AbstractInterpreter::layout_activation(Method* method,
   intptr_t* sender_sp;
   if (caller->is_interpreted_frame()) {
     sender_sp = caller->interpreter_frame_top_frame_sp();
+#ifdef ASSERT
+    assert(locals_base <= caller->interpreter_frame_expression_stack(), "bad placement");
+    // Test caller-aligned placement vs callee-aligned
+    intptr_t* l2 = (caller->sp() + method->max_locals() - 1 +
+      frame::z_parent_ijava_frame_abi_size / Interpreter::stackElementSize);
+    assert(locals_base >= l2, "bad placement");
+#endif
   } else if (caller->is_compiled_frame()) {
     sender_sp = caller->fp() - caller->cb()->frame_size();
     // The bottom frame's sender_sp is its caller's unextended_sp.

src/hotspot/cpu/x86/abstractInterpreter_x86.cpp

@@ -87,7 +87,10 @@ void AbstractInterpreter::layout_activation(Method* method,
 
 #ifdef ASSERT
   if (caller->is_interpreted_frame()) {
-    assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
+    // Test exact placement on top of caller args
+    intptr_t* l2 = caller->interpreter_frame_last_sp() + caller_actual_parameters - 1;
+    assert(l2 <= caller->interpreter_frame_expression_stack(), "bad placement");
+    assert(l2 >= locals, "bad placement");
   }
 #endif
 

src/hotspot/share/interpreter/bytecode.hpp

@@ -226,6 +226,8 @@ class Bytecode_invoke: public Bytecode_member_ref {
 
   bool has_appendix();
 
+  bool has_member_arg() const;
+
   int size_of_parameters() const;
 
  private:

src/hotspot/share/interpreter/bytecode.inline.hpp

@@ -28,6 +28,7 @@
 #include "interpreter/bytecode.hpp"
 
 #include "oops/cpCache.inline.hpp"
+#include "prims/methodHandles.hpp"
 
 inline bool Bytecode_invoke::has_appendix() {
   if (invoke_code() == Bytecodes::_invokedynamic) {
@@ -37,4 +38,13 @@ inline bool Bytecode_invoke::has_appendix() {
   }
 }
 
+inline bool Bytecode_invoke::has_member_arg() const {
+  // NOTE: We could resolve the call and use the resolved adapter method here, but this function
+  // is used by deoptimization, where resolving could lead to problems, so we avoid that here
+  // by doing things symbolically.
+  //
+  // invokedynamic instructions don't have a class but obviously don't have a MemberName appendix.
+  return !is_invokedynamic() && MethodHandles::has_member_arg(klass(), name());
+}
+
 #endif // SHARE_INTERPRETER_BYTECODE_INLINE_HPP

src/hotspot/share/runtime/deoptimization.cpp

@@ -36,6 +36,7 @@
 #include "gc/shared/collectedHeap.hpp"
 #include "gc/shared/memAllocator.hpp"
 #include "interpreter/bytecode.hpp"
+#include "interpreter/bytecode.inline.hpp"
 #include "interpreter/bytecodeStream.hpp"
 #include "interpreter/interpreter.hpp"
 #include "interpreter/oopMapCache.hpp"
@@ -641,11 +642,12 @@ Deoptimization::UnrollBlock* Deoptimization::fetch_unroll_info_helper(JavaThread
   bool caller_was_method_handle = false;
   if (deopt_sender.is_interpreted_frame()) {
     methodHandle method(current, deopt_sender.interpreter_frame_method());
-    Bytecode_invoke cur = Bytecode_invoke_check(method, deopt_sender.interpreter_frame_bci());
-    if (cur.is_invokedynamic() || cur.is_invokehandle()) {
-      // Method handle invokes may involve fairly arbitrary chains of
-      // calls so it's impossible to know how much actual space the
-      // caller has for locals.
+    Bytecode_invoke cur(method, deopt_sender.interpreter_frame_bci());
+    if (cur.has_member_arg()) {
+      // This should cover all real-world cases.  One exception is a pathological chain of
+      // MH.linkToXXX() linker calls, which only trusted code could do anyway.  To handle that case, we
+      // would need to get the size from the resolved method entry.  Another exception would
+      // be an invokedynamic with an adapter that is really a MethodHandle linker.
       caller_was_method_handle = true;
     }
   }
@@ -748,9 +750,14 @@ Deoptimization::UnrollBlock* Deoptimization::fetch_unroll_info_helper(JavaThread
   }
 #endif
 
+  int caller_actual_parameters = -1; // value not used except for interpreted frames, see below
+  if (deopt_sender.is_interpreted_frame()) {
+    caller_actual_parameters = callee_parameters + (caller_was_method_handle ? 1 : 0);
+  }
+
   UnrollBlock* info = new UnrollBlock(array->frame_size() * BytesPerWord,
                                       caller_adjustment * BytesPerWord,
-                                      caller_was_method_handle ? 0 : callee_parameters,
+                                      caller_actual_parameters,
                                       number_of_frames,
                                       frame_sizes,
                                       frame_pcs,
@@ -939,7 +946,7 @@ JRT_LEAF(BasicType, Deoptimization::unpack_frames(JavaThread* thread, int exec_m
       if (Bytecodes::is_invoke(cur_code)) {
         Bytecode_invoke invoke(mh, iframe->interpreter_frame_bci());
         cur_invoke_parameter_size = invoke.size_of_parameters();
-        if (i != 0 && !invoke.is_invokedynamic() && MethodHandles::has_member_arg(invoke.klass(), invoke.name())) {
+        if (i != 0 && invoke.has_member_arg()) {
           callee_size_of_parameters++;
         }
       }

src/hotspot/share/runtime/vframeArray.cpp

@@ -25,6 +25,7 @@
 #include "classfile/vmSymbols.hpp"
 #include "code/vmreg.inline.hpp"
 #include "interpreter/bytecode.hpp"
+#include "interpreter/bytecode.inline.hpp"
 #include "interpreter/interpreter.hpp"
 #include "memory/allocation.inline.hpp"
 #include "memory/resourceArea.hpp"
@@ -610,10 +611,7 @@ void vframeArray::unpack_to_stack(frame &unpack_frame, int exec_mode, int caller
       methodHandle caller(current, elem->method());
       methodHandle callee(current, element(index - 1)->method());
       Bytecode_invoke inv(caller, elem->bci());
-      // invokedynamic instructions don't have a class but obviously don't have a MemberName appendix.
-      // NOTE:  Use machinery here that avoids resolving of any kind.
-      const bool has_member_arg =
-          !inv.is_invokedynamic() && MethodHandles::has_member_arg(inv.klass(), inv.name());
+      const bool has_member_arg = inv.has_member_arg();
       callee_parameters = callee->size_of_parameters() + (has_member_arg ? 1 : 0);
       callee_locals     = callee->max_locals();
     }