8257828: SafeFetch may crash if invoked in non-JavaThreads

tstuefe · tstuefe · commit 3ab1dfeb8fc2 · 2020-12-15T07:00:54.000Z
Reviewed-by: mdoerr, kbarrett, coleenp, dholmes
diff --git a/src/hotspot/os/posix/signals_posix.cpp b/src/hotspot/os/posix/signals_posix.cpp
@@ -32,12 +32,19 @@
 #include "runtime/java.hpp"
 #include "runtime/os.hpp"
 #include "runtime/osThread.hpp"
+#include "runtime/stubRoutines.hpp"
 #include "runtime/thread.hpp"
 #include "signals_posix.hpp"
 #include "utilities/events.hpp"
 #include "utilities/ostream.hpp"
 #include "utilities/vmError.hpp"
 
+#ifdef ZERO
+// See stubGenerator_zero.cpp
+#include <setjmp.h>
+extern sigjmp_buf* get_jmp_buf_for_continuation();
+#endif
+
 #include <signal.h>
 
 // Various signal related mechanism are laid out in the following order:
@@ -546,13 +553,36 @@ int JVM_HANDLE_XXX_SIGNAL(int sig, siginfo_t* info,
 
   // Handle assertion poison page accesses.
 #ifdef CAN_SHOW_REGISTERS_ON_ASSERT
-  if ((sig == SIGSEGV || sig == SIGBUS) && info != NULL && info->si_addr == g_assert_poison) {
+  if (!signal_was_handled &&
+      ((sig == SIGSEGV || sig == SIGBUS) && info != NULL && info->si_addr == g_assert_poison)) {
     signal_was_handled = handle_assert_poison_fault(ucVoid, info->si_addr);
   }
 #endif
 
+  if (!signal_was_handled) {
+    // Handle SafeFetch access.
+#ifndef ZERO
+    if (uc != NULL) {
+      address pc = os::Posix::ucontext_get_pc(uc);
+      if (StubRoutines::is_safefetch_fault(pc)) {
+        os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
+        signal_was_handled = true;
+      }
+    }
+#else
+    // See JDK-8076185
+    if (sig == SIGSEGV || sig == SIGBUS) {
+      sigjmp_buf* const pjb = get_jmp_buf_for_continuation();
+      if (pjb) {
+        siglongjmp(*pjb, 1);
+      }
+    }
+#endif // ZERO
+  }
+
   // Ignore SIGPIPE and SIGXFSZ (4229104, 6499219).
-  if (sig == SIGPIPE || sig == SIGXFSZ) {
+  if (!signal_was_handled &&
+      (sig == SIGPIPE || sig == SIGXFSZ)) {
     PosixSignals::chained_handler(sig, info, ucVoid);
     signal_was_handled = true; // unconditionally.
   }
diff --git a/src/hotspot/os/windows/os_windows.cpp b/src/hotspot/os/windows/os_windows.cpp
@@ -2242,29 +2242,24 @@ int os::signal_wait() {
 LONG Handle_Exception(struct _EXCEPTION_POINTERS* exceptionInfo,
                       address handler) {
   Thread* thread = Thread::current_or_null();
-  // Save pc in thread
+
 #if defined(_M_ARM64)
-  // Do not blow up if no thread info available.
-  if (thread) {
-    thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->Pc);
-  }
-  // Set pc to handler
-  exceptionInfo->ContextRecord->Pc = (DWORD64)handler;
+  #define PC_NAME Pc
 #elif defined(_M_AMD64)
-  // Do not blow up if no thread info available.
-  if (thread != NULL) {
-    thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->Rip);
-  }
-  // Set pc to handler
-  exceptionInfo->ContextRecord->Rip = (DWORD64)handler;
+  #define PC_NAME Rip
+#elif defined(_M_IX86)
+  #define PC_NAME Eip
 #else
-  // Do not blow up if no thread info available.
-  if (thread != NULL) {
-    thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->Eip);
+  #error unknown architecture
+#endif
+
+  // Save pc in thread
+  if (thread != nullptr && thread->is_Java_thread()) {
+    thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->PC_NAME);
   }
+
   // Set pc to handler
-  exceptionInfo->ContextRecord->Eip = (DWORD)(DWORD_PTR)handler;
-#endif
+  exceptionInfo->ContextRecord->PC_NAME = (DWORD64)handler;
 
   // Continue the execution
   return EXCEPTION_CONTINUE_EXECUTION;
diff --git a/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp b/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
@@ -180,17 +180,6 @@ bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
   // retrieve crash address
   address const addr = info ? (const address) info->si_addr : NULL;
 
-  // SafeFetch 32 handling:
-  // - make it work if _thread is null
-  // - make it use the standard os::...::ucontext_get/set_pc APIs
-  if (uc) {
-    address const pc = os::Posix::ucontext_get_pc(uc);
-    if (pc && StubRoutines::is_safefetch_fault(pc)) {
-      os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
-      return true;
-    }
-  }
-
   if (info == NULL || uc == NULL) {
     return false; // Fatal error
   }
diff --git a/src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp b/src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp
@@ -411,11 +411,6 @@ bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
   if (info != NULL && uc != NULL && thread != NULL) {
     pc = (address) os::Posix::ucontext_get_pc(uc);
 
-    if (StubRoutines::is_safefetch_fault(pc)) {
-      os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
-      return true;
-    }
-
     // Handle ALL stack overflow variations here
     if (sig == SIGSEGV || sig == SIGBUS) {
       address addr = (address) info->si_addr;
diff --git a/src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp b/src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp
@@ -57,10 +57,6 @@
 #include "utilities/events.hpp"
 #include "utilities/vmError.hpp"
 
-// See stubGenerator_zero.cpp
-#include <setjmp.h>
-extern sigjmp_buf* get_jmp_buf_for_continuation();
-
 address os::current_stack_pointer() {
   address dummy = (address) &dummy;
   return dummy;
@@ -118,14 +114,6 @@ frame os::fetch_frame_from_context(const void* ucVoid) {
 bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
                                              ucontext_t* uc, JavaThread* thread) {
 
-  // handle SafeFetch faults the zero way
-  if (sig == SIGSEGV || sig == SIGBUS) {
-    sigjmp_buf* const pjb = get_jmp_buf_for_continuation();
-    if (pjb) {
-      siglongjmp(*pjb, 1);
-    }
-  }
-
   if (info != NULL && thread != NULL) {
     // Handle ALL stack overflow variations here
     if (sig == SIGSEGV || sig == SIGBUS) {
diff --git a/src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp b/src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp
@@ -185,11 +185,6 @@ bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
   if (info != NULL && uc != NULL && thread != NULL) {
     pc = (address) os::Posix::ucontext_get_pc(uc);
 
-    if (StubRoutines::is_safefetch_fault(pc)) {
-      os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
-      return true;
-    }
-
     address addr = (address) info->si_addr;
 
     // Make sure the high order byte is sign extended, as it may be masked away by the hardware.
diff --git a/src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp b/src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp
@@ -268,10 +268,6 @@ bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
     if (sig == SIGSEGV) {
       address addr = (address) info->si_addr;
 
-      if (StubRoutines::is_safefetch_fault(pc)) {
-        os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
-        return true;
-      }
       // check if fault address is within thread stack
       if (thread->is_in_full_stack(addr)) {
         // stack overflow
diff --git a/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp b/src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp
@@ -213,16 +213,6 @@ bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
     }
   }
 
-  // Moved SafeFetch32 handling outside thread!=NULL conditional block to make
-  // it work if no associated JavaThread object exists.
-  if (uc) {
-    address const pc = os::Posix::ucontext_get_pc(uc);
-    if (pc && StubRoutines::is_safefetch_fault(pc)) {
-      os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
-      return true;
-    }
-  }
-
   // decide if this trap can be handled by a stub
   address stub = NULL;
   address pc   = NULL;
diff --git a/src/hotspot/os_cpu/linux_s390/os_linux_s390.cpp b/src/hotspot/os_cpu/linux_s390/os_linux_s390.cpp
@@ -207,16 +207,6 @@ frame os::current_frame() {
 bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
                                              ucontext_t* uc, JavaThread* thread) {
 
-  // Moved SafeFetch32 handling outside thread!=NULL conditional block to make
-  // it work if no associated JavaThread object exists.
-  if (uc) {
-    address const pc = os::Posix::ucontext_get_pc(uc);
-    if (pc && StubRoutines::is_safefetch_fault(pc)) {
-      os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
-      return true;
-    }
-  }
-
   // Decide if this trap can be handled by a stub.
   address stub    = NULL;
   address pc      = NULL;  // Pc as retrieved from PSW. Usually points past failing instruction.
diff --git a/src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp b/src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp
@@ -221,11 +221,6 @@ bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
   if (info != NULL && uc != NULL && thread != NULL) {
     pc = (address) os::Posix::ucontext_get_pc(uc);
 
-    if (StubRoutines::is_safefetch_fault(pc)) {
-      os::Posix::ucontext_set_pc(uc, StubRoutines::continuation_for_safefetch_fault(pc));
-      return true;
-    }
-
 #ifndef AMD64
     // Halt if SI_KERNEL before more crashes get misdiagnosed as Java bugs
     // This can happen in any running code (currently more frequently in
diff --git a/src/hotspot/os_cpu/linux_zero/os_linux_zero.cpp b/src/hotspot/os_cpu/linux_zero/os_linux_zero.cpp
@@ -53,10 +53,6 @@
 #include "utilities/events.hpp"
 #include "utilities/vmError.hpp"
 
-// See stubGenerator_zero.cpp
-#include <setjmp.h>
-extern sigjmp_buf* get_jmp_buf_for_continuation();
-
 address os::current_stack_pointer() {
   // return the address of the current function
   return (address)__builtin_frame_address(0);
@@ -114,14 +110,6 @@ frame os::fetch_frame_from_context(const void* ucVoid) {
 bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
                                              ucontext_t* uc, JavaThread* thread) {
 
-  // handle SafeFetch faults
-  if (sig == SIGSEGV || sig == SIGBUS) {
-    sigjmp_buf* const pjb = get_jmp_buf_for_continuation();
-    if (pjb) {
-      siglongjmp(*pjb, 1);
-    }
-  }
-
   if (info != NULL && thread != NULL) {
     // Handle ALL stack overflow variations here
     if (sig == SIGSEGV) {
diff --git a/test/hotspot/gtest/runtime/test_safefetch.cpp b/test/hotspot/gtest/runtime/test_safefetch.cpp
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020 SAP SE. 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include "precompiled.hpp"
+#include "runtime/interfaceSupport.inline.hpp"
+#include "runtime/stubRoutines.hpp"
+#include "runtime/vmOperations.hpp"
+#include "runtime/vmThread.hpp"
+#include "utilities/globalDefinitions.hpp"
+#include "unittest.hpp"
+
+static const intptr_t pattern = LP64_ONLY(0xABCDABCDABCDABCDULL) NOT_LP64(0xABCDABCD);
+static intptr_t* invalid_address = (intptr_t*)(intptr_t) NOT_AIX(os::min_page_size()) AIX_ONLY(-1);
+
+TEST_VM(os, safefetch_positive) {
+  intptr_t v = pattern;
+  intptr_t a = SafeFetchN(&v, 1);
+  ASSERT_EQ(v, a);
+}
+
+#ifndef _WIN32
+// Needs JDK-8185734 to be solved
+TEST_VM(os, safefetch_negative) {
+  intptr_t a = SafeFetchN(invalid_address, pattern);
+  ASSERT_EQ(pattern, a);
+  a = SafeFetchN(invalid_address, ~pattern);
+  ASSERT_EQ(~pattern, a);
+}
+#endif // _WIN32
+
+class VM_TestSafeFetchAtSafePoint : public VM_GTestExecuteAtSafepoint {
+public:
+  void doit() {
+    // Regression test for JDK-8257828
+    // Should not crash.
+    intptr_t a = SafeFetchN(invalid_address, pattern);
+    ASSERT_EQ(pattern, a);
+    a = SafeFetchN(invalid_address, ~pattern);
+    ASSERT_EQ(~pattern, a);
+  }
+};
+
+TEST_VM(os, safefetch_negative_at_safepoint) {
+  VM_TestSafeFetchAtSafePoint op;
+  ThreadInVMfromNative invm(JavaThread::current());
+  VMThread::execute(&op);
+}
