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

[David Holmes]

Hi Dean,

I fixed the cc from core-libs-dev-request at openjdk.java.net to 
core-libs-dev at openjdk.java.net

On 16/03/2017 7:28 AM, dean.long at oracle.com wrote:
> https://bugs.openjdk.java.net/browse/JDK-8158168
>
> http://cr.openjdk.java.net/~dlong/8158168/

Not a full review sorry, just a couple of comments.

src/share/vm/classfile/javaClasses.hpp

Given the the only call to java_lang_String::set_debug_intrinsics is 
within an ifdef, shouldn't the declaration and definition of the method 
also be guarded the same way?


> 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.

The changes to the JDK core classes are quite extensive. This will need 
rigorous functional and performance testing and it is very late in the 
release cycle to make these kinds of changes. But I'll leave that to the 
core-libs folk to comment on.

David
-----

> 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.  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.
>
> See the bug report for tests run.
>
> There are some unavoidable performance regressions on micro benchmarks,
> because now we are doing bounds checks that we weren't before.
>
> dl
>