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

[Vladimir Ivanov]

> and webrev.2 with it removed:
>
> http://cr.openjdk.java.net/~dlong/8158168/webrev.2/

Thanks, Dean. I started with webrev.2 and tried to minimize the changes. 
I ended up with the following version:

   http://cr.openjdk.java.net/~vlivanov/dlong/8158168/webrev.00/

Some clarifications:

============
src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals when it 
extracts info from AbstractStringBuilder. I don't see how out of bounds 
access can happen in String.contentEquals:
          if (n != length()) {
              return false;
          }
...
              for (int i = 0; i < n; i++) {
                  if (StringUTF16.getChar(val, i) != cs.charAt(i)) {

============
src/java.base/share/classes/java/lang/StringConcatHelper.java:

I think bounds checks in StringConcatHelper.prepend() are skipped 
intentionally, since java.lang.invoke.StringConcatFactory constructs 
method handle chains which already contain bounds checks: array length 
is precomputed based on argument values and all accesses are guaranteed 
to be in bounds.

============
src/java.base/share/classes/java/lang/StringUTF16.java:

+    static void putChar(byte[] val, int index, int c) {
+        assert index >= 0 && index < length(val) : "Trusted caller 
missed bounds check";

Unfortunately, asserts can affect inlining decisions (since they 
increase bytecode size). In order to minimize possible performance 
impact, I suggest to remove them from the fix targeting 9.

============
      private static int indexOfSupplementary(byte[] value, int ch, int 
fromIndex, int max) {
          if (Character.isValidCodePoint(ch)) {
              final char hi = Character.highSurrogate(ch);
              final char lo = Character.lowSurrogate(ch);
+            checkBoundsBeginEnd(fromIndex, max, value);

The check is redundant here. fromIndex & max are always inbounds by 
construction:

     public static int indexOf(byte[] value, int ch, int fromIndex) {
         int max = value.length >> 1;
         if (fromIndex < 0) {
             fromIndex = 0;
         } else if (fromIndex >= max) {
             // Note: fromIndex might be near -1>>>1.
             return -1;
         }
...
             return indexOfSupplementary(value, ch, fromIndex, max);

============
I moved bounds checks from StringUTF16.lastIndexOf/indexOf to 
ABS.indexOf/lastIndexOf. I think it's enough to do range check on 
ABS.value & ABS.count. After that, all accesses should be inbounds by 
construction (in String.indexOf/lastIndexOf):

jdk/src/java.base/share/classes/java/lang/StringUTF16.java:
     static int lastIndexOf(byte[] src, byte srcCoder, int srcCount,
                            String tgtStr, int fromIndex) {

         int rightIndex = srcCount - tgtCount;
         if (fromIndex > rightIndex) {
             fromIndex = rightIndex;
         }
         if (fromIndex < 0) {
             return -1;
         }

jdk/src/java.base/share/classes/java/lang/StringUTF16.java:
     public static int lastIndexOf(byte[] src, int srcCount,
                                   byte[] tgt, int tgtCount, int 
fromIndex) {
         int min = tgtCount - 1;
         int i = min + fromIndex;
         int strLastIndex = tgtCount - 1;
         char strLastChar = getChar(tgt, strLastIndex);

     startSearchForLastChar:
         while (true) {
             while (i >= min && getChar(src, i) != strLastChar) {

There are 2 places:
   * getChar(tgt, strLastIndex) => getChar(tgt, tgtCount-1) - inbound

   * getChar(src, i); i in [ min; min+fromIndex ]
	min = tgtCount - 1
	rightIndex = srcCount - tgtCount
	fromIndex <= rightIndex

        	0 <= min + fromIndex <= min + rightIndex == (tgtCount - 1) + 
(srcCount - tgtCount) == srcCount - 1

     Hence, should be covered by the check on count & value:
       public int lastIndexOf(String str, int fromIndex) {
+         byte[] value = this.value;
+         int count = this.count;
+         byte coder = this.coder;
+         checkIndex(count, value.length >> coder);
           return String.lastIndexOf(value, coder, count, str, fromIndex);
       }

Best regards,
Vladimir Ivanov

> On 3/17/17 5:58 AM, Vladimir Ivanov wrote:
>>
>>>> I have the same concern. Can we fix the immediate problem in 9 and
>>>> integrate verification logic in 10?
>>>>
>>>
>>> OK, Tobias is suggesting having verification logic only inside the
>>> intrinsics.  Are you suggesting removing that as well?
>>
>> Yes and put them back in 10.
>>
>>> I'm OK with removing all the verification, but that won't reduce the
>>> library changes much.  I could undo the renaming to Trusted.getChar, but
>>> we would still have the bounds checks moved into StringUTF16.
>>
>> I suggest to go with a point fix for 9: just add missing range checks.
>