RFR: JDK-8257828: SafeFetch may crash if invoked in non-JavaThreads

[Coleen Phillimore]

On Tue, 8 Dec 2020 13:35:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> In our primary hotspot signal handlers, SafeFetch handling is limited to JavaThread objects:
> 
>   JavaThread* thread = NULL;
> ...
>   if(t->is_Java_thread()) {
>     thread = (JavaThread*)t;
>   }
> ...
>   if (info != NULL && uc != NULL && thread != NULL) {
>     pc = (address) os::Linux::ucontext_get_pc(uc);
>     if (StubRoutines::is_safefetch_fault(pc)) {
> 
> As a result of this, using SafeFetch may crash non-JavaThreads if the location is invalid. E.g. using SafeFetch inside a VMOperation may crash the VM.
> 
> This is unfortunate since SafeFetch is used for os::is_readable_pointer() which explicitly promises to not crash. It is used e.g. in os::print_hex_dump(). There is also no reason why SafeFetch would not work for non-JavaThreads. In fact, SafeFetch handling for the secondary signal handler works just fine for all threads.
> 
> ----
> 
> The patch makes handling of SafeFetch faults independent on whether the crashing thread is a JavaThread (indeed, whether we have a current Thread at all). This had been the case for AIX and Linux ppc, s390 before, since we already fixed this issue for our platform, so we know this works.
> 
> I also hauled the SafeFetch handling out of the platform dependent part of the signal handler into the generic signal handler. This removes some duplicate coding.
> 
> To be consistent, I moved the SafeFetch handling for Zero up into the generic signal handler too. Zero did not have a problem, but this reduces code.
> 
> I added a gtest which reproduces the issue and used that to check that the patch works.
> 
> Thanks, Thomas

This looks good to me.  Moving this to the posix signal handler is so nice.

-------------

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1695