On Fri, 21 Oct 2022 03:37:16 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org>
wrote:
>> DefaultListSelectionModel.removeIndexInterva accepts `int` value which
>> allows it to take in Integer.MAX_VALUE theoratically but it does calculation
>> with that value which can results in IOOBE.
>> Fix is to make sure the calculation stays within bounds.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> removeIndexInterval, insertIndexInterval fix
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
653:
> 651: if (length < 0 || index < 0) {
> 652: return;
> 653: }
Throw IOOBE as it's done in other methods. Silently ignoring bad parameters
isn't good, likely it means an error in the calling code.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
656:
> 654: if (index + length < 0) {
> 655: return;
> 656: }
Other methods, like `removeIndexInterval`, accept parameters which result in
integer overflow, I think this should allow it too. Yet you have to ensure
`index + length` isn't greater than `Integer.MAX_VALUE`. I mean `length` should
be normalized in such a case, so that it doesn't cause the overflow.
Something like `length = Integer.MAX_VALUE - index`, it's off the top of my
head, I haven't verified it's correct, it's just an idea on how this should be
handled.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
669:
> 667: if (length + maxIndex < 0) {
> 668: maxIndex = Integer.MAX_VALUE - length;
> 669: }
You must not change `maxIndex` this way — you just broke the class invariant:
**`maxIndex` points to the highest selected index**. Now it doesn't.
You have to make sure `i + length <= Integer.MAX_VALUE` but you must not modify
`maxIndex` directly. This would probably be handled automatically if you
normalize `length`. If not, the body of `if` may need a condition.
Alternatively, a better way, the start value of `i` should be reduced so that
is always `i + length <= Integer.MAX_VALUE` and doesn't cause integer overflow.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
735:
> 733: setState(i, (i <= Integer.MAX_VALUE - gapLength)
> 734: && (i + gapLength >= minIndex)
> 735: && value.get(i + gapLength));
I would prefer if the `&&` operators are aligned to the opening parenthesis of
the first condition, it makes it clearer that the condition is part of the
second parameter. I don't insist on this one, the wrapping style is not agreed
on.
-------------
PR: https://git.openjdk.org/jdk/pull/10409
