[9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[Tobias Hartmann]

Hi Dean,

On 15.03.2017 22:28, dean.long at oracle.com wrote:
> http://cr.openjdk.java.net/~dlong/8158168/
> 
> This crash is caused by missing array bounds checks on compact string intrinsics.  It shows up when unsynchronized access to a StringBuilder object causes inconsistent field values.
> 
> To convince myself that all the necessary bounds checks are being done, I put callers into two groups, trusted and untrusted. Untrusted callers are all directed through StringUTF16 methods, so that bounds checks are done in one place and can be tested easily. Trusted callers bypass the bounds checks, so they must do their own checking.
> 
> As a safety net, I added asserts around the intrinsic calls, and a try/catch that so any out of bounds exception turns into an assert error as well.  

So the assert and try/catch are only necessary to catch invalid offsets passed to the C1 intrinsic, right? Interpreted code is safe and the C2 intrinsics have additional guards in debug builds.

I'm fine with that but an alternative would be to also have such guards in the C1 intrinsic. For example, one could enable the normal bound checks and add a simple check to Runtime1::throw_range_check_exception() that fails if the throwing method is the C1 intrinsic. Like this, we could avoid the assert, try-catch and DEBUG_INTRINSICS code.

> Finally, I restored some C2 debug code that was previously removed, and I use it to do bounds checking in debug builds.  In a product build C2 will remove all of these.

To clarify: we introduced those bound checks during Compact Strings development for additional verification and removed them before integration:
http://hg.openjdk.java.net/jdk9/sandbox/hotspot/rev/8a7f17b4709f

It may make sense re-introduce these checks also for other intrinsics in JDK 10 to catch similar problems. I'll look at it with JDK-8156534.

I noticed these unused imports in StringUTF16.java:
 38 //import static java.lang.String.checkIndex;
 39 //import static java.lang.String.checkOffset;
 40 //import static java.lang.String.checkBoundsOffCount;

Maybe add a comment to the DEBUG_INTRINSICS declaration.

Thanks,
Tobias