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