ppkarwasz commented on code in PR #756:
URL: https://github.com/apache/commons-compress/pull/756#discussion_r2598458600
##########
src/main/java/org/apache/commons/compress/utils/SeekableInMemoryByteChannel.java:
##########
@@ -113,8 +113,8 @@ public long position() throws ClosedChannelException {
@Override
public SeekableByteChannel position(final long newPosition) throws
IOException {
ensureOpen();
- if (newPosition < 0L || newPosition > Integer.MAX_VALUE) {
- throw new IllegalArgumentException(String.format("Position must be
in range [0..%,d]: %,d", Integer.MAX_VALUE, newPosition));
+ if (newPosition < 0L) {
+ throw new IllegalArgumentException(String.format("New position is
negative: %,d", newPosition));
}
position = (int) newPosition;
Review Comment:
I am afraid that after this change, the `position` field must be promoted to
a `long` to prevent an integer overflow.
By removing the check `position > Integer.MAX_VALUE` here, we should
introduce it in:
- At the beginning of `read`: if `position` is greater than
`Integer.MAX_VALUE` then `-1` is immediately returned.
- At the beginning of `write`: if `position` is greater than
`Integer.MAX_VALUE` a `IOException` must be thrown.
Moreover, I saw a related bug in `truncate`: if the argument is more than
`Integer.MAX_VALUE`, the method should be a no-op.
##########
src/test/java/org/apache/commons/compress/utils/SeekableInMemoryByteChannelTest.java:
##########
@@ -152,9 +152,11 @@ void testShouldThrowExceptionOnWritingToClosedChannel() {
}
@Test
- void testShouldThrowExceptionWhenSettingIncorrectPosition() {
+ void testShouldThrowExceptionWhenSettingIncorrectPosition() throws
IOException {
Review Comment:
Should we have different tests here?
- If the argument is `-1`, an exception should be thrown,
- If the argument is `2L << 32 - 1L` (same as `-1` when cast to an `int`),
no exception should be thrown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
