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

[David Holmes]

On 22/03/2017 10:26 AM, dean.long at oracle.com wrote:
> On 3/21/17 5:02 PM, David Holmes wrote:
>
>> 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?
>>
>
> 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 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.

Thanks,
David
-----

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