[9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8
dean.long at oracle.com
dean.long at oracle.com
Thu Mar 23 18:25:47 UTC 2017
On 3/22/17 1:49 PM, Vladimir Ivanov wrote:
>> Also, it looks like the changes I made to ASB.appendChars(char[] s, int
>> off, int end) are not needed.
>
> Agree.
>
>>> Vladimir, don't you need to replace checkIndex with checkOffset in
>>> indexOf and lastIndexOf, so that we allow count == length?
>
> Yes, my bad. Good catch. Updated webrev in place.
>
> FTR I haven't done any extensive testing of the minimized fix.
>
> If we agree to proceed with it, the regression test should be updated
> as well. I think the viable solution would be to construct broken SBs
> (using reflection) and invoke affected methods on them.
>
We can construct broken SBs using the Helper class that gets patched
into java.lang. I'll work on that.
dl
> Best regards,
> Vladimir Ivanov
>
>>> 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.
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>>
More information about the hotspot-compiler-dev
mailing list