Skip to content

Commit 208887b

Browse files
garydgregorygarydgregory
authored
ByteArraySeekableByteChannel.position|truncate(long) shouldn't throw an IllegalArgumentException for a new positive position that's too large (#817)
* [IO-856] Try test on all OSs for GitHub CI * ByteArraySeekableByteChannel.position|truncate(long) shouldn't throw an IllegalArgumentException for a new positive position that's too large * Throw IOException instead of OutOfMemoryError * Refactor internals for safer and less type casting
1 parent 3557766 commit 208887b

File tree

4 files changed

+106
-30
lines changed

4 files changed

+106
-30
lines changed

‎src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java‎

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public static ByteArraySeekableByteChannel wrap(final byte[] bytes) {
6868

6969
private byte[] data;
7070
private volatile boolean closed;
71-
private int position;
71+
private long position;
7272
private int size;
7373
private final ReentrantLock lock = new ReentrantLock();
7474

@@ -126,11 +126,10 @@ private void checkOpen() throws ClosedChannelException {
126126
}
127127
}
128128

129-
private int checkRange(final long newSize, final String method) {
130-
if (newSize < 0L || newSize > IOUtils.SOFT_MAX_ARRAY_LENGTH) {
131-
throw new IllegalArgumentException(String.format("%s must be in range [0..%,d]: %,d", method, IOUtils.SOFT_MAX_ARRAY_LENGTH, newSize));
129+
private void checkRange(final long newSize, final String method) {
130+
if (newSize < 0L) {
131+
throw new IllegalArgumentException(String.format("%s must be positive: %,d", method, newSize));
132132
}
133-
return (int) newSize;
134133
}
135134

136135
@Override
@@ -166,10 +165,10 @@ public long position() throws ClosedChannelException {
166165
@Override
167166
public SeekableByteChannel position(final long newPosition) throws IOException {
168167
checkOpen();
169-
final int intPos = checkRange(newPosition, "position()");
168+
checkRange(newPosition, "position()");
170169
lock.lock();
171170
try {
172-
position = intPos;
171+
position = newPosition;
173172
} finally {
174173
lock.unlock();
175174
}
@@ -181,15 +180,18 @@ public int read(final ByteBuffer buf) throws IOException {
181180
checkOpen();
182181
lock.lock();
183182
try {
183+
if (position > Integer.MAX_VALUE) {
184+
return IOUtils.EOF;
185+
}
184186
int wanted = buf.remaining();
185-
final int possible = size - position;
187+
final int possible = size - (int) position;
186188
if (possible <= 0) {
187189
return IOUtils.EOF;
188190
}
189191
if (wanted > possible) {
190192
wanted = possible;
191193
}
192-
buf.put(data, position, wanted);
194+
buf.put(data, (int) position, wanted);
193195
position += wanted;
194196
return wanted;
195197
} finally {
@@ -238,14 +240,14 @@ public byte[] toByteArray() {
238240
@Override
239241
public SeekableByteChannel truncate(final long newSize) throws ClosedChannelException {
240242
checkOpen();
241-
final int intSize = checkRange(newSize, "truncate()");
243+
checkRange(newSize, "truncate()");
242244
lock.lock();
243245
try {
244-
if (size > intSize) {
245-
size = intSize;
246+
if (size > newSize) {
247+
size = (int) newSize;
246248
}
247-
if (position > intSize) {
248-
position = intSize;
249+
if (position > newSize) {
250+
position = newSize;
249251
}
250252
} finally {
251253
lock.unlock();
@@ -256,21 +258,28 @@ public SeekableByteChannel truncate(final long newSize) throws ClosedChannelExce
256258
@Override
257259
public int write(final ByteBuffer b) throws IOException {
258260
checkOpen();
261+
if (position > Integer.MAX_VALUE) {
262+
throw new IOException("position > Integer.MAX_VALUE");
263+
}
259264
lock.lock();
260265
try {
261266
final int wanted = b.remaining();
262-
final int possibleWithoutResize = Math.max(0, size - position);
263-
if (wanted > possibleWithoutResize) {
264-
final int newSize = position + wanted;
265-
if (newSize < 0 || newSize > IOUtils.SOFT_MAX_ARRAY_LENGTH) { // overflow
266-
throw new OutOfMemoryError("required array size " + Integer.toUnsignedString(newSize) + " too large");
267-
}
268-
resize(newSize);
267+
// intPos <= Integer.MAX_VALUE
268+
final int intPos = (int) position;
269+
final long newPosition = position + wanted;
270+
if (newPosition > IOUtils.SOFT_MAX_ARRAY_LENGTH) {
271+
throw new IOException(String.format("Requested array size %,d is too large.", newPosition));
269272
}
270-
b.get(data, position, wanted);
271-
position += wanted;
272-
if (size < position) {
273-
size = position;
273+
if (newPosition > size) {
274+
final int newPositionInt = (int) newPosition;
275+
// Ensure that newPositionInt ≤ data.length
276+
resize(newPositionInt);
277+
size = newPositionInt;
278+
}
279+
b.get(data, intPos, wanted);
280+
position = newPosition;
281+
if (size < intPos) {
282+
size = intPos;
274283
}
275284
return wanted;
276285
} finally {

‎src/test/java/org/apache/commons/io/channels/AbstractSeekableByteChannelTest.java‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747
*/
4848
abstract class AbstractSeekableByteChannelTest {
4949

50-
private SeekableByteChannel channel;
50+
protected SeekableByteChannel channel;
5151

5252
@TempDir
5353
protected Path tempDir;
@@ -87,6 +87,7 @@ void testCloseMultipleTimes() throws IOException {
8787
assertFalse(channel.isOpen());
8888
}
8989

90+
9091
@Test
9192
void testConcurrentPositionAndSizeQueries() throws IOException {
9293
final byte[] data = "test data".getBytes();
@@ -136,6 +137,20 @@ void testPositionBeyondSize() throws IOException {
136137
assertEquals(4, channel.size()); // Size should not change
137138
}
138139

140+
@Test
141+
void testPositionBeyondSizeRead() throws IOException {
142+
final ByteBuffer buffer = ByteBuffer.allocate(1);
143+
channel.position(channel.size() + 1);
144+
assertEquals(channel.size() + 1, channel.position());
145+
assertEquals(-1, channel.read(buffer));
146+
channel.position(Integer.MAX_VALUE + 1L);
147+
assertEquals(Integer.MAX_VALUE + 1L, channel.position());
148+
assertEquals(-1, channel.read(buffer));
149+
assertThrows(IllegalArgumentException.class, () -> channel.position(-1));
150+
assertThrows(IllegalArgumentException.class, () -> channel.position(Integer.MIN_VALUE));
151+
assertThrows(IllegalArgumentException.class, () -> channel.position(Long.MIN_VALUE));
152+
}
153+
139154
@ParameterizedTest
140155
@CsvSource({ "0, 0", "5, 5", "10, 10", "100, 100" })
141156
void testPositionInBounds(final long newPosition, final long expectedPosition) throws IOException {
@@ -149,6 +164,7 @@ void testPositionInBounds(final long newPosition, final long expectedPosition) t
149164
assertEquals(expectedPosition, channel.position());
150165
}
151166

167+
152168
@Test
153169
void testPositionNegative() {
154170
assertThrows(IllegalArgumentException.class, () -> channel.position(-1));
@@ -292,6 +308,18 @@ void testSizeSameOnOverwrite() throws IOException {
292308
assertEquals(11, channel.size()); // Size should not change
293309
}
294310

311+
@Test
312+
void testTrucateBeyondSizeReadWrite() throws IOException {
313+
final ByteBuffer buffer = ByteBuffer.allocate(1);
314+
channel.truncate(channel.size() + 1);
315+
assertEquals(-1, channel.read(buffer));
316+
channel.truncate(Integer.MAX_VALUE + 1L);
317+
assertEquals(-1, channel.read(buffer));
318+
assertThrows(IllegalArgumentException.class, () -> channel.truncate(-1));
319+
assertThrows(IllegalArgumentException.class, () -> channel.truncate(Integer.MIN_VALUE));
320+
assertThrows(IllegalArgumentException.class, () -> channel.truncate(Long.MIN_VALUE));
321+
}
322+
295323
@Test
296324
void testTruncateNegative() {
297325
assertThrows(IllegalArgumentException.class, () -> channel.truncate(-1));

‎src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -162,16 +162,39 @@ void testShouldThrowExceptionOnWritingToClosedChannel() {
162162
}
163163

164164
@Test
165-
void testShouldThrowExceptionWhenSettingIncorrectPosition() {
165+
void testThrowWhenSettingIncorrectPosition() throws IOException {
166166
try (ByteArraySeekableByteChannel c = new ByteArraySeekableByteChannel()) {
167-
assertThrows(IllegalArgumentException.class, () -> c.position(Integer.MAX_VALUE + 1L));
167+
final ByteBuffer buffer = ByteBuffer.allocate(1);
168+
// write
169+
c.write(buffer);
170+
assertEquals(1, c.position());
171+
// bad pos A
172+
c.position(c.size() + 1);
173+
assertEquals(c.size() + 1, c.position());
174+
assertEquals(-1, c.read(buffer));
175+
// bad pos B
176+
c.position(Integer.MAX_VALUE + 1L);
177+
assertEquals(Integer.MAX_VALUE + 1L, c.position());
178+
assertEquals(-1, c.read(buffer));
179+
assertThrows(IOException.class, () -> c.write(buffer));
180+
// negative input is the only illegal input
181+
assertThrows(IllegalArgumentException.class, () -> c.position(-1));
182+
assertThrows(IllegalArgumentException.class, () -> c.position(Integer.MIN_VALUE));
183+
assertThrows(IllegalArgumentException.class, () -> c.position(Long.MIN_VALUE));
168184
}
169185
}
170186

171187
@Test
172-
void testShouldThrowExceptionWhenTruncatingToIncorrectSize() {
188+
void testThrowWhenTruncatingToIncorrectSize() throws IOException {
173189
try (ByteArraySeekableByteChannel c = new ByteArraySeekableByteChannel()) {
174-
assertThrows(IllegalArgumentException.class, () -> c.truncate(Integer.MAX_VALUE + 1L));
190+
final ByteBuffer buffer = ByteBuffer.allocate(1);
191+
c.truncate(c.size() + 1);
192+
assertEquals(-1, c.read(buffer));
193+
c.truncate(Integer.MAX_VALUE + 1L);
194+
assertEquals(-1, c.read(buffer));
195+
assertThrows(IllegalArgumentException.class, () -> c.truncate(-1));
196+
assertThrows(IllegalArgumentException.class, () -> c.truncate(Integer.MIN_VALUE));
197+
assertThrows(IllegalArgumentException.class, () -> c.truncate(Long.MIN_VALUE));
175198
}
176199
}
177200

‎src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelTest.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,22 @@ void testConstructorInvalid() {
8484
assertThrows(NullPointerException.class, () -> ByteArraySeekableByteChannel.wrap(null));
8585
}
8686

87+
@Test
88+
void testPositionBeyondSizeReadWrite() throws IOException {
89+
final ByteBuffer buffer = ByteBuffer.allocate(1);
90+
channel.position(channel.size() + 1);
91+
assertEquals(channel.size() + 1, channel.position());
92+
assertEquals(-1, channel.read(buffer));
93+
channel.position(Integer.MAX_VALUE + 1L);
94+
assertEquals(Integer.MAX_VALUE + 1L, channel.position());
95+
assertEquals(-1, channel.read(buffer));
96+
// ByteArraySeekableByteChannel has a hard boundary at Integer.MAX_VALUE, files don't.
97+
assertThrows(IOException.class, () -> channel.write(buffer));
98+
assertThrows(IllegalArgumentException.class, () -> channel.position(-1));
99+
assertThrows(IllegalArgumentException.class, () -> channel.position(Integer.MIN_VALUE));
100+
assertThrows(IllegalArgumentException.class, () -> channel.position(Long.MIN_VALUE));
101+
}
102+
87103
@ParameterizedTest
88104
@MethodSource
89105
void testShouldResizeWhenWritingMoreDataThanCapacity(final byte[] data, final int wanted) throws IOException {

0 commit comments

Comments
 (0)