On Fri, 14 Oct 2022 09:16:27 GMT, Prasanta Sadhukhan <[email protected]>
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:
>
> javadoc update
I was wrong, disallowing `Integer.MAX_VALUE` for `setSelectionInterval` is
**not enough**. Moreover, if `Integer.MAX_VALUE` isn't accepted for
`setSelectionInterval` and `removeIndexInterval`, it should be rejected in
other places too. Otherwise,
`selectionModel.isSelectedIndex(Integer.MAX_VALUE))` being valid looks
inconsistent.
Even when the `gapLength` remains positive, the following loop could throw
IOOBE because a negative index being accessed. The following code
selectionModel.setSelectionInterval(Integer.MAX_VALUE - 2,
Integer.MAX_VALUE - 1);
selectionModel.removeIndexInterval(0, Integer.MAX_VALUE - 1);
throws
Exception in thread "main" java.lang.IndexOutOfBoundsException: bitIndex < 0:
-2147483648
at java.base/java.util.BitSet.get(BitSet.java:626)
at
java.desktop/javax.swing.DefaultListSelectionModel.removeIndexInterval(DefaultListSelectionModel.java:713)
at SelectionModelTest.main(SelectionModelTest.java)
where line 713
https://github.com/openjdk/jdk/blob/ee43bd8db4c378c8cecdf5051cbaa6ac177d3592/src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java#L713
I suggested the code to fix that problem.
Then the infinite loop in `changeSelection`
https://github.com/openjdk/jdk/blob/ee43bd8db4c378c8cecdf5051cbaa6ac177d3592/src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java#L432
for `MAX_VALUE` is resolved by reversing it:
- for(int i = Math.min(setMin, clearMin); i <= Math.max(setMax,
clearMax); i++) {
+ for(int i = Math.max(setMax, clearMax); i >= Math.min(setMin,
clearMin); i--) {
The proposed changes will resolve the bug.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
707:
> 705: int rmMinIndex = Math.min(index0, index1);
> 706: int rmMaxIndex = Math.max(index0, index1);
> 707: int gapLength = (rmMaxIndex - rmMinIndex) + 1;
Suggestion:
int gapLength = (rmMaxIndex - rmMinIndex) == Integer.MAX_VALUE
? Integer.MAX_VALUE
: (rmMaxIndex - rmMinIndex) + 1;
This ensures `gapLength` remains positive. (The condition `(rmMaxIndex -
rmMinIndex) == Integer.MAX_VALUE` is `true` if and only if `rmMinIndex == 0 &&
rmMaxIndex == Integer.MAX_VALUE`.)
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
714:
> 712: for(int i = rmMinIndex; i <= maxIndex; i++) {
> 713: setState(i, value.get(i + gapLength));
> 714: }
The `for`-loop should look like this:
for(int i = rmMinIndex; i >= 0 && i <= maxIndex; i++) {
setState(i, (i <= Integer.MAX_VALUE - gapLength)
&& (i + gapLength >= minIndex)
&& value.get(i + gapLength));
}
It breaks if `i` becomes negative. In the body, the first condition `(i <=
Integer.MAX_VALUE - gapLength)` prevents accessing indexes greater than
`Integer.MAX_VALUE`, the second condition `(i + gapLength >= minIndex)` is an
optimisation, the values for indexes below `minIndex` are `false`, so the call
to `value.get` is skipped. (The second condition is not required.)
-------------
Changes requested by aivanov (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10409
