[10]RFR: 6415680: (bf) MappedByteBuffer.get() can provoke crash with EXCEPTION_IN_PAGE_ERROR

[David Holmes]

On 15/11/2017 4:27 AM, dean.long at oracle.com wrote:
> Adding runtime alias...

Thanks Dean!

> comments inlined below.
> 
> 
> On 11/13/17 9:10 PM, jamsheed wrote:
> 
>> Hi, request for review, jbs: 
>> https://bugs.openjdk.java.net/browse/JDK-6415680 webrev: 
>> http://cr.openjdk.java.net/~jcm/6415680/webrev.00/ Description: 1) 
>> changes equivalent to JDK-4454115 is done for windows.
> 
> It looks like "nm" can be uninitialized if "in_java" is false.
> 
>> 2) added guard to multiple value access sites, Unsafe_CopyMemory0, 
>> Unsafe_SetMemory0 and Unsafe_CopySwapMemory0.
> 
> Can you narrow the scope of the unsafe access using something like 
> GuardUnsafeAccess, instead of marking the whole function as doing unsafe 
> access?

I tend to agree with this suggestion. The unsafe region should be as 
narrow as possible.

> There are some risks with trying to  abort a copy function.
> 
> First, won't we get multiple exceptions until we finally hit the end of 
> the range?  What if the bad range is very large?
> 
> Second, what if the loop is using auto-increment instructions? Skipping 
> to the next instruction would mean we loop forever if the increment 
> never happens.
> 
> I think if we are going to safely abort copy functions then we need to 
> register them as a kind of CodeBlob that has a special abort entry point 
> or exception handler we can redirect to, or maybe pop the whole frame 
> and return.

Jamsheed and I has some discussions on this on IM last week. I also 
flagged the multiple exceptions as potentially problematic. I'm not sure 
how things will behave if we trigger hundreds of faults in succession - 
nor how long it will take. Aborting the whole loop, or frame, seems 
preferable but I don't know how you would do that.

> Is there really a problem with these copy functions?  I'm wondering why 
> Mikael did not identify these as a problem in 8154592.

Good question. :) It seems quite obvious to me that if a single 
load/store needs protection then bulk load/store would also need 
protection. And Jamsheeds tests confirmed that.

Cheers,
David

>> 3) Unsafe_CopySwapMemory0 is JVM_LEAF so removed 
>> thread->thread_state() == _thread_in_vm checks from signal handler
> 
> How about adding a check for _thread_in_native instead of removing the 
> check entirely?
> 
> dl
> 
>> Best regards, Jamsheed
>>
>