[9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed Mar 22 20:49:07 UTC 2017
> 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.
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-dev
mailing list