RFR: 6187113: DefaultListSelectionModel.removeIndexInterval(0, Integer.MAX_VALUE) fails [v5]

[Alexey Ivanov]

On Fri, 14 Oct 2022 09:16:27 GMT, Prasanta Sadhukhan <psadhukhan at 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:
> 
>   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