New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
6187113: DefaultListSelectionModel.removeIndexInterval(0, Integer.MAX_VALUE) fails #10409
Conversation
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
Webrevs
|
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
CSR raised..https://bugs.openjdk.org/browse/JDK-8295329...please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
setState(i, value.get(i + gapLength)); |
I suggested the code to fix that problem.
Then the infinite loop in changeSelection
for(int i = Math.min(setMin, clearMin); i <= Math.max(setMax, clearMax); i++) { |
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
Show resolved
Hide resolved
for(int i = rmMinIndex; i <= maxIndex; i++) { | ||
if ((i + gapLength) > gapLength) { | ||
setState(i, value.get(i + gapLength)); | ||
} else { | ||
setState(i, value.get(gapLength)); | ||
} | ||
setState(i, value.get(i + gapLength)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There exists at least one other problem because of integer overflow in This code selectionModel.setSelectionInterval(Integer.MAX_VALUE - 1, Integer.MAX_VALUE);
selectionModel.insertIndexInterval(Integer.MAX_VALUE - 1, Integer.MAX_VALUE, true); throws Exception in thread "main" java.lang.IndexOutOfBoundsException: bitIndex < 0: -2
at java.base/java.util.BitSet.get(BitSet.java:626)
at java.desktop/javax.swing.DefaultListSelectionModel.set(DefaultListSelectionModel.java:311)
at java.desktop/javax.swing.DefaultListSelectionModel.setState(DefaultListSelectionModel.java:629)
at java.desktop/javax.swing.DefaultListSelectionModel.insertIndexInterval(DefaultListSelectionModel.java:657)
at SelectionModelTest.main(SelectionModelTest.java) Would you like to include it as part of this fix? Shall I submit a new bug for this? |
Thanks for your suggestive code.. BTW, your code change resulted in JCK failure so I have used a mix of what I had in previous iterations and your code and this satisfied JCK as well as the unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tough. The number of test cases has grown to 25. There are many failures.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
if ((i + length) >= length) { | ||
setState(i + length, value.get(i)); | ||
} else { | ||
setState(i, value.get(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operation in the else
block doesn't make sense, you can just skip it.
@@ -654,7 +658,11 @@ public void insertIndexInterval(int index, int length, boolean before) | |||
* insMinIndex). | |||
*/ | |||
for(int i = maxIndex; i >= insMinIndex; i--) { | |||
setState(i + length, value.get(i)); | |||
if ((i + length) >= length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using i <= Integer.MAX_VALUE - length
. Yet I'm still unsure it's enough and correct.
@@ -654,7 +658,11 @@ public void insertIndexInterval(int index, int length, boolean before) | |||
* insMinIndex). | |||
*/ | |||
for(int i = maxIndex; i >= insMinIndex; i--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insertIndexInterval
doesn't verify its parameters: negative index
, negative length
should be rejected right away.
if ((i + gapLength) >= gapLength) { | ||
setState(i, value.get(i + gapLength)); | ||
} else { | ||
setState(i, value.get(gapLength)); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work. It must not break. With this code, 5 of my tests fail.
For example,
selectionModel.setSelectionInterval(0, Integer.MAX_VALUE);
selectionModel.removeIndexInterval(0, Integer.MAX_VALUE);
must result in empty selection. All the possible indexes were selected, you removed all the possible indexes (so that “the list” contains no elements at all). This test case fails either way, unfortunately: selectionModel.isSelectedIndex(0)
still returns true
.
This edge case where rmMinIndex = 0
and rmMaxIndex = Integer.MAX_VALUE
should probably be short-circuited: setState(i, false)
for all the elements. Other values are handled correctly by my suggested code, as far as I can see.
This test case:
selectionModel.setSelectionInterval(0, Integer.MAX_VALUE);
selectionModel.removeIndexInterval(1, Integer.MAX_VALUE);
passed before and now it fails again. Here, selectionModel.isSelectedIndex(0)
must be true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we handle the edge case separately, it looks better and the first test case as well as other tests for remove
pass successfully:
public void removeIndexInterval(int index0, int index1)
{
if (index0 < 0 || index1 < 0) {
throw new IndexOutOfBoundsException("index is negative");
}
int rmMinIndex = Math.min(index0, index1);
int rmMaxIndex = Math.max(index0, index1);
if (rmMinIndex == 0 && rmMaxIndex == Integer.MAX_VALUE) {
for (int i = Integer.MAX_VALUE; i >= 0; i--) {
setState(i, false);
}
// min and max are updated automatically by the for-loop
// TODO Update anchor and lead
return;
}
int gapLength = (rmMaxIndex - rmMinIndex) + 1;
/* Shift the entire bitset to the left to close the index0, index1
* gap.
*/
for(int i = rmMinIndex; i >= 0 && i <= maxIndex; i++) {
setState(i, (i <= Integer.MAX_VALUE - gapLength)
&& (i + gapLength >= minIndex)
&& value.get(i + gapLength));
}
// The rest of the method
}
selectionModel.setSelectionInterval(Integer.MAX_VALUE - 1, Integer.MAX_VALUE); | ||
selectionModel.insertIndexInterval(Integer.MAX_VALUE - 1, Integer.MAX_VALUE, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think testing that it does not throw an exception is not enough. You should verify that the data selection model holds is correct.
According to the spec, the interval from Integer.MAX_VALUE - 1
to Integer.MAX_VALUE
should remain selected.
if (length < 0 || index < 0) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (index + length < 0) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (length + maxIndex < 0) { | ||
maxIndex = Integer.MAX_VALUE - length; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
setState(i, (i <= Integer.MAX_VALUE - gapLength) | ||
&& (i + gapLength >= minIndex) | ||
&& value.get(i + gapLength)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already mentioned that I wrote a comprehensive unit-test to test the changes to removeIndexInterval
and insertIndexInterval
. The latest version of my test SelectionModelTest.java
.
With the current code in the PR, six (6) test cases still fail. The changes I've suggested should resolve those failures. (I've been testing and proposing fixes to resolve test failures since the start of this code review.)
The test can be run as a jtreg test or as a standalone app. Depending on the hardware, it may take up to 10 minutes; the more cores the system has, the faster the test completes as all the test cases are run in parallel. (The minimum time I've ever seen is around 2–3 minutes.)
I can contribute the test to OpenJDK if deemed useful. I believe such a fix can't be done without extensive testing which verifies all the values in the object remain correct.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
boolean setInsertedValues = ((getSelectionMode() == SINGLE_SELECTION) ? | ||
false : value.get(index)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose resolving an IDE warning and simplifying this condition:
boolean setInsertedValues = ((getSelectionMode() == SINGLE_SELECTION) ? | |
false : value.get(index)); | |
boolean setInsertedValues = (getSelectionMode() != SINGLE_SELECTION | |
&& value.get(index)); |
Although this change is unrelated.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java
Outdated
Show resolved
Hide resolved
@@ -449,6 +449,10 @@ private void changeSelection(int clearMin, int clearMax, | |||
if (shouldClear) { | |||
clear(i); | |||
} | |||
// Integer overlfow | |||
if (i + 1 < i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (i + 1 < i) { | |
if (i == Integer.MAX_VALUE) { |
Will it be clearer this way? (And one less operation: no addition.)
As I mentioned before, I would prefer reversing the loop and iterating from max
to min
instead. But it causes a failure in JCK which I can't explain because the effective result is the same. Moreover, I believe iterating from max
to min
is more efficient as touching the maximum index will ensure the internal storage in the underlying BitSet
is allocated right away to fit all the bits whereas the storage gets continuously re-allocated and copied as the accessed indexes grow.
*/ | ||
import javax.swing.DefaultListSelectionModel; | ||
|
||
public class TestDefListModelException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose moving this test to JList
folder which already contains several tests which use ListSelectionModel
via JList
.
The failure of Other five failures are valid ones, |
Let's keep it open. CSR review has stalled. |
@prsadhuk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
The CSR was approved two days ago, yet the bots didn't update the status of the issue. It should be ready for integration. |
/label add csr |
@prsadhuk
|
/csr |
/csr JDK-8295329 |
/csr unneeded |
@prsadhuk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
/csr needed |
@prsadhuk has indicated that a compatibility and specification (CSR) request is needed for this pull request. @prsadhuk please create a CSR request for issue JDK-6187113 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
/integrate |
Going to push as commit c2ebd17. |
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.
Progress
Issues
Reviewers
Contributors
<aivanov@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10409/head:pull/10409
$ git checkout pull/10409
Update a local copy of the PR:
$ git checkout pull/10409
$ git pull https://git.openjdk.org/jdk pull/10409/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10409
View PR using the GUI difftool:
$ git pr show -t 10409
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10409.diff