Improving parameter checking in Apache Harmony

I'm going through some Apache Harmony code at the moment, and spotting a few places where there is a common pattern of code that needs fixing.

Consider this method:

 public synchronized int read(byte[] target, int offset, int length) throws IOException {
   if (length + offset > target.length || length < 0 || offset < 0) {
       throw new ArrayIndexOutOfBoundsException();
   }
   if (0 == length) {
       return 0;
   }
   ByteBuffer buffer = ByteBuffer.wrap(target, offset, length);
   return channel.read(buffer);
 }

Can you see the problem?
Hint: the parameter checking in the if() statement is wrong.

The original author rightly wants to ensure that the offset and length represent plausible values for the data to be put into the buffer; that is, the method should not attempt to place data outside the bounds of the 'target' byte array.

The problem is that in Java, like a number of other languages, (int) values overflow without any exception being thrown. In the code above we are checking for illegal values by computing (length + offset) and throwing an exception if it is greater than the buffer's length. This ensures we don't "write off the end" of the target buffer. However, if the sum overflows an int representation then it may indeed pass this safety check, and permit a value for offset or length that is too large.

As a practical illustration, consider that the 'offset' actual parameter value is Integer.MAX_VALUE = (int)2147483647, and the 'length' value is, say 42. In this case the sum of these positive values is a negative value '(int)(2147483647 + 42) = (int)-2147483607' so the less-than test passes and the method is broken!

The fix is to avoid the sum, and convert it into a subtraction, e.g.

if (length < 0 || offset < 0 || length > target.length - offset) {
    throw new ArrayIndexOutOfBoundsException();
}

Now we can be assured that there will be no int overflow. The target.length cannot be less than zero, so the subtraction can only produce values in the range (0 - Integer.MAX_VALUE = Integer.MIN_VALUE + 1) to (Integer.MAX_VALUE - 0 = Integer.MAX_VALUE).

I know this is not news for Java developers. Josh Bloch and others have been highlighting the problem for a number of years, yet I've found about four places in the code so far that exhibit this potential problem. They'll all be fixed in time for the next milestone build.