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

[Kim Barrett]

On Sat, 12 Dec 2020 07:17:14 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
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make SafeFetch work on Windows + AIX fix

New Windows changes look okay to me, but I'm not an expert in that area.

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

Marked as reviewed by kbarrett (Reviewer).

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