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

[dean.long at oracle.com]

Vladimir, don't you need to replace checkIndex with checkOffset in 
indexOf and lastIndexOf, so that we allow count == length?

dl


On 3/22/17 8:35 AM, Vladimir Ivanov wrote:
>>>> 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?
>>>
>>> If only we had a static analysis tool that could tell us if the code is
>>> safe.  Because we don't, in my initial changeset, we always take a
>>> snapshot of the ASB fields by passing those field values to StringUTF16
>>> before doing checks on them.  And I wrote a test to make sure that 
>>> those
>>> StringUTF16 interfaces are catching all the underflows and overflows I
>>> could imagine, and I added verification code to detect when a check was
>>> missed.
>>>
>>> However, all the reviewers have requested to minimize the amount of
>>> changes.  In Vladimir's version, if there is a missing check somewhere,
>>> then yes it could lead to a crash.
>
> I'd like to point out that asserts and verification code are disabled 
> by default. They are invaluable during problem diagnosis, but don't 
> help at all from defence-in-depth perspective.
>
> But I agree that it's easier to reason about and test the initial 
> version of the fix.
>
>> I wonder if the reviewers have fully realized the potential impact here?
>> This has exposed a flaw in the way intrinsics are used from core 
>> classes.
>
> FTR here are the checks I omitted in the minimized version (modulo 
> separation of indexOf/lastIndexOf for trusted/non-trusted callers):
>
> http://cr.openjdk.java.net/~vlivanov/dlong/8158168/redundant_checks/
>
> Other than that, the difference is mainly about undoing refactorings 
> and removing verification logic (asserts + checks in the JVM).
>
> There are still unsafe accesses which are considered safe in both 
> versions (see StringUTF16.Trusted usages in the initial version [1]).
>
> We used to provide safe wrappers for unsafe intrinsics which makes it 
> much easier to reason about code correctness. I'd like to see compact 
> string code refactored that way and IMO the initial version by Dean is 
> a big step in the right direction.
>
> I still prefer to see a point fix in 9 and major refactoring happening 
> in 10, but I'll leave the decision on how to proceed with the fix to 
> core-libs folks. After finishing the exercise minimizing the fix, I'm 
> much more comfortable with the initial fix [1] (though there are 
> changes I consider excessive).
>
> Best regards,
> Vladimir Ivanov
>
> [1] http://cr.openjdk.java.net/~dlong/8158168/webrev.0
>
>>>>>> 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.
>>>>>>>
>>>>>
>>>