ppkarwasz commented on code in PR #817:
URL: https://github.com/apache/commons-io/pull/817#discussion_r2599899272
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -256,21 +258,27 @@ public SeekableByteChannel truncate(final long newSize)
throws ClosedChannelExce
@Override
public int write(final ByteBuffer b) throws IOException {
checkOpen();
+ if (position > Integer.MAX_VALUE) {
+ throw new IOException("position > Integer.MAX_VALUE");
+ }
lock.lock();
try {
final int wanted = b.remaining();
- final int possibleWithoutResize = Math.max(0, size - position);
+ // intPos <= Integer.MAX_VALUE
+ int intPos = (int) position;
+ final int possibleWithoutResize = Math.max(0, size - intPos);
if (wanted > possibleWithoutResize) {
- final int newSize = position + wanted;
+ final int newSize = intPos + wanted;
if (newSize < 0 || newSize > IOUtils.SOFT_MAX_ARRAY_LENGTH) {
// overflow
throw new OutOfMemoryError("required array size " +
Integer.toUnsignedString(newSize) + " too large");
}
resize(newSize);
}
Review Comment:
I am wondering if we shouldn't at the same time transform this
`OutOfMemoryError` into an `IOException`.
I also considered improving the code to perform partial writes as we go
closer to the limit, but apparently the
[Javadoc](https://docs.oracle.com/javase/8/docs/api/java/nio/channels/WritableByteChannel.html)
does not allow that:
> Unless otherwise specified, a write operation will return only after
writing all of the `r` requested bytes.
Therefore the condition on `newSize` is correct: we can **not** write less
than requested.
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -256,21 +258,27 @@ public SeekableByteChannel truncate(final long newSize)
throws ClosedChannelExce
@Override
public int write(final ByteBuffer b) throws IOException {
checkOpen();
+ if (position > Integer.MAX_VALUE) {
+ throw new IOException("position > Integer.MAX_VALUE");
+ }
lock.lock();
try {
final int wanted = b.remaining();
- final int possibleWithoutResize = Math.max(0, size - position);
+ // intPos <= Integer.MAX_VALUE
+ int intPos = (int) position;
+ final int possibleWithoutResize = Math.max(0, size - intPos);
if (wanted > possibleWithoutResize) {
- final int newSize = position + wanted;
+ final int newSize = intPos + wanted;
if (newSize < 0 || newSize > IOUtils.SOFT_MAX_ARRAY_LENGTH) {
// overflow
throw new OutOfMemoryError("required array size " +
Integer.toUnsignedString(newSize) + " too large");
}
resize(newSize);
}
- b.get(data, position, wanted);
- position += wanted;
- if (size < position) {
- size = position;
+ b.get(data, intPos, wanted);
+ // intPos + wanted is at most (Integer.MAX_VALUE - intPos) + intPos
+ position = intPos += wanted;
Review Comment:
I am really _nitpicking_ this, but I noticed that the new value of
`position` is exactly the value of `newSize` above…
I was thinking about something like that:
```java
final long newPosition = position + wanted;
if (newPosition > IOUtils.SOFT_MAX_ARRAY_LENGTH) {
throw new IOException("required array size " + newPosition + "too
large");
}
if (newPosition > size) {
final int newPositionInt = (int) newPosition;
// Ensure that newPositionInt ≤ data.length
resize(newPositionInt);
size = newPositionInt;
}
b.get(data, (int) position, wanted);
position = newPosition;
```
where the casts are safe, since `position ≤ newPosition ≤
IOUtils.SOFT_MAX_ARRAY_LENGTH`.
--
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]
