RFR: 6187113: DefaultListSelectionModel.removeIndexInterval(0, Integer.MAX_VALUE) fails [v7]
Alexey Ivanov
aivanov at openjdk.org
Fri Oct 21 21:27:51 UTC 2022
On Fri, 21 Oct 2022 03:37:16 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:
>
> 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
More information about the client-libs-dev
mailing list