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

David Holmes david.holmes at oracle.com
Wed Mar 22 05:09:40 UTC 2017

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.

More information about the hotspot-dev mailing list