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

[dean.long at oracle.com]

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.

OK.

>
>> 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.
>
> Is AbstractStringBuilder.append() the only affected method? (Sorry, 
> it's hard to say exactly where the problem is by looking at the diff.)
>

In the failing test, yes, it was append, but when I went to fix the 
problem I found that it was much more wide spread, and there were 
several methods that were affected.

> I really like the refactoring you propose on jdk side, but there are 
> pieces I'm not sure about. For example, I spotted a repeated range check:
>
> jdk/src/java.base/share/classes/java/lang/AbstractStringBuilder.java:
>     public void setCharAt(int index, char ch) {
>         checkIndex(index, count);
>         if (isLatin1() && StringLatin1.canEncode(ch)) {
>             value[index] = (byte)ch;
>         } else {
>             if (isLatin1()) {
>                 inflate();
>             }
>             StringUTF16.putCharSB(value, index, ch);
>         }
>     }
>

OK, I did not look for redundant checks.  This check is actually not 
redundant.  The "value" array may be oversized, so "count" is supposed 
to contain the current maximum.  For the safety of the intrinsic array 
access, we check against the array length, but for the API we need to be 
stricter and check against the character count.

However, the checkIndex() here is a good example of what is wrong. Let's 
say we were checking against value.length instead of "count". Even if 
checkIndex() succeeds here, based on the current length of "value", we 
can't trust it because the object is mutable and "value" can change 
between the checkIndex() and putCharSB().

dl

> Best regards,
> Vladimir Ivanov