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

[David Holmes]

I haven't been looking at the details of this but have been watching 
from afar. As per my comments in the bug report (now public) I'm quite 
concerned about the thread-non-safety issue here ...

On 22/03/2017 4:47 AM, dean.long at oracle.com wrote:
> On 3/21/17 9:37 AM, Vladimir Ivanov wrote:
>
>>> 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/
>>
>
> Thanks.  The reason I didn't go with that approach from the beginning is
> because I couldn't convince myself that I could find all the missing
> bounds checks, and I wanted an interface to test against.  With the
> bounds checks in AbstractStringBuilder, it is very hard to test all the
> possible race conditions, because some of the race conditions only
> happen when an ASB field changes half-way through the method.

So are we convinced that the proposed changes will never lead to a crash 
due to a missing or incorrect bounds check, due to a racy use of an 
unsynchronized ASB instance e.g. StringBuilder?

Thanks,
David
-----

>> 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)) {
>>
>
> OK.
>
>> ============
>> 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.
>>
>
> This is calling the trusted version of getChars() with no bounds
> checks.  It was a little more obvious when I had the Trusted inner class.
>
>> ============
>> 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.
>>
>
> Sure.
>
>> ============
>>      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);
>>
>
> OK.
>
>> ============
>> 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);
>>       }
>>
>
> OK, I will go with your version if it's OK with Sherman.
>
> dl
>
>> 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.
>>>
>