On Fri, 14 Oct 2022 09:16:27 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: > > 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