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

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Tue Mar 21 16:37:00 UTC 2017

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:

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);

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:

(srcCount - tgtCount) == srcCount - 1

 Hence, should be covered by the check on count & value:
   public int lastIndexOf(String str, int 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.

More information about the hotspot-dev mailing list