[PATCH] 6788196: Array bounds checks in io_util.c rely on undefined behaviour
Martin Buchholz
martinrb at google.com
Wed Jan 7 01:02:38 UTC 2009
Hi Gary,
I will be your submitter for this fix.
I don't think it's worth adding checks for array length being non-negative.
We can add tests to the existing test case
jdk/test/java/io/readBytes/ReadBytesBounds.java
I am providing a patch which has a revised patch and test.
Please forgive me for having refactored everything.
I created an outOfBounds function, suitable for copying and pasting.
I propose to commit this, if Gary and Alan give me thumbs up.
The patch to the test is hard to read.
public class ReadBytesBounds {
static final FileInputStream fis;
static final RandomAccessFile raf;
static final byte[] b = new byte[32];
static {
try {
String dir = System.getProperty("test.src", ".");
File testFile = new File(dir, "input.txt");
fis = new FileInputStream(testFile);
raf = new RandomAccessFile(testFile , "r");
} catch (Throwable t) {
throw new Error(t);
}
}
public static void main(String argv[]) throws Throwable {
byte b[] = new byte[32];
testRead(-1, -1, false);
testRead(-1, 0, false);
testRead( 0, -1, false);
testRead( 0, 33, false);
testRead(33, 0, false);
testRead(33, 4, false);
testRead( 0, 32, true);
testRead(32, 0, true);
testRead(32, 4, false);
testRead( 4, 16, true);
testRead( 1, 31, true);
testRead( 0, 0, true);
testRead(31, Integer.MAX_VALUE, false);
testRead( 0, Integer.MAX_VALUE, false);
testRead(-1, Integer.MAX_VALUE, false);
testRead(-4, Integer.MIN_VALUE, false);
testRead( 0, Integer.MIN_VALUE, false);
}
static void testRead(int off, int len, boolean expected) throws Throwable {
System.err.printf("off=%d len=%d expected=%b%n", off, len, expected);
boolean result;
try {
fis.read(b, off, len);
raf.read(b, off, len);
result = true;
} catch (IndexOutOfBoundsException e) {
result = false;
}
if (result != expected) {
throw new RuntimeException
(String.format("Unexpected result off=%d len=%d expected=%b",
off, len, expected));
}
}
}
Martin
On Tue, Jan 6, 2009 at 08:38, Gary Benson <gbenson at redhat.com> wrote:
> Hi Martin,
>
> I like your method of avoiding the overflow, it's a nice idea.
> I've attached an updated version of my original patch, with that,
> and with an expanded comment too, to make sure the fix doesn't
> get reverted later on in the interests of readability or whatever.
>
> Can I ask that you file a seperate bug for your other changes?
> They're not specifically related to 6788196, and I feel it
> confuses the issue somewhat having a bunch of unrelated changes
> in the patch.
>
> Cheers,
> Gary
>
> Martin Buchholz wrote:
>> I have sympathy for Gary's reluctance to use jlong,
>> even though we all know that here the performance of 64-bit
>> operands doesn't matter.
>>
>> I propose an alternate patch, which avoids the overflow problem
>> by simply rearranging the operands, and adds other pedantic
>> improvements.
>>
>> I believe it may not be 100% portable to replace
>> (len < 0) || (off < 0)
>> with
>> ((len | off) < 0)
>> I'll leave that optimization to the compiler.
>>
>> diff --git a/src/share/native/java/io/io_util.c
>> b/src/share/native/java/io/io_util.c
>> --- a/src/share/native/java/io/io_util.c
>> +++ b/src/share/native/java/io/io_util.c
>> @@ -25,6 +25,7 @@
>>
>> #include <stdlib.h>
>> #include <string.h>
>> +#include <stddef.h>
>>
>> #include "jni.h"
>> #include "jni_util.h"
>> @@ -34,9 +35,9 @@
>>
>> /* IO helper functions */
>>
>> -int
>> +jint
>> readSingle(JNIEnv *env, jobject this, jfieldID fid) {
>> - int nread;
>> + jint nread;
>> char ret;
>> FD fd = GET_FD(this, fid);
>> if (fd == -1) {
>> @@ -59,13 +60,14 @@ readSingle(JNIEnv *env, jobject this, jf
>> #define BUF_SIZE 8192
>>
>>
>> -int
>> +jint
>> readBytes(JNIEnv *env, jobject this, jbyteArray bytes,
>> jint off, jint len, jfieldID fid)
>> {
>> - int nread, datalen;
>> + jint nread;
>> + jsize datalen;
>> char stackBuf[BUF_SIZE];
>> - char *buf = 0;
>> + char *buf = NULL;
>> FD fd;
>>
>> if (IS_NULL(bytes)) {
>> @@ -74,8 +76,10 @@ readBytes(JNIEnv *env, jobject this, jby
>> }
>> datalen = (*env)->GetArrayLength(env, bytes);
>>
>> - if ((off < 0) || (off > datalen) ||
>> - (len < 0) || ((off + len) > datalen) || ((off + len) < 0)) {
>> + if ((off < 0) ||
>> + (len < 0) ||
>> + /* Avoid undefined signed integer overflow. */
>> + (datalen - off < len)) {
>> JNU_ThrowByName(env, "java/lang/IndexOutOfBoundsException", 0);
>> return -1;
>> }
>> @@ -84,7 +88,7 @@ readBytes(JNIEnv *env, jobject this, jby
>> return 0;
>> } else if (len > BUF_SIZE) {
>> buf = malloc(len);
>> - if (buf == 0) {
>> + if (buf == NULL) {
>> JNU_ThrowOutOfMemoryError(env, 0);
>> return 0;
>> }
>> @@ -118,7 +122,7 @@ void
>> void
>> writeSingle(JNIEnv *env, jobject this, jint byte, jfieldID fid) {
>> char c = byte;
>> - int n;
>> + jint n;
>> FD fd = GET_FD(this, fid);
>> if (fd == -1) {
>> JNU_ThrowIOException(env, "Stream Closed");
>> @@ -134,11 +138,12 @@ writeSingle(JNIEnv *env, jobject this, j
>>
>> void
>> writeBytes(JNIEnv *env, jobject this, jbyteArray bytes,
>> - jint off, jint len, jfieldID fid)
>> + jint off, jint len, jfieldID fid)
>> {
>> - int n, datalen;
>> + jint n;
>> + jsize datalen;
>> char stackBuf[BUF_SIZE];
>> - char *buf = 0;
>> + char *buf = NULL;
>> FD fd;
>>
>> if (IS_NULL(bytes)) {
>> @@ -147,8 +152,10 @@ writeBytes(JNIEnv *env, jobject this, jb
>> }
>> datalen = (*env)->GetArrayLength(env, bytes);
>>
>> - if ((off < 0) || (off > datalen) ||
>> - (len < 0) || ((off + len) > datalen) || ((off + len) < 0)) {
>> + if ((off < 0) ||
>> + (len < 0) ||
>> + /* Avoid undefined signed integer overflow. */
>> + (datalen - off < len)) {
>> JNU_ThrowByName(env, "java/lang/IndexOutOfBoundsException", 0);
>> return;
>> }
>> @@ -157,7 +164,7 @@ writeBytes(JNIEnv *env, jobject this, jb
>> return;
>> } else if (len > BUF_SIZE) {
>> buf = malloc(len);
>> - if (buf == 0) {
>> + if (buf == NULL) {
>> JNU_ThrowOutOfMemoryError(env, 0);
>> return;
>> }
>> @@ -196,19 +203,19 @@ throwFileNotFoundException(JNIEnv *env,
>> throwFileNotFoundException(JNIEnv *env, jstring path)
>> {
>> char buf[256];
>> - int n;
>> + jint n;
>> jobject x;
>> jstring why = NULL;
>>
>> n = JVM_GetLastErrorString(buf, sizeof(buf));
>> if (n > 0) {
>> - why = JNU_NewStringPlatform(env, buf);
>> + why = JNU_NewStringPlatform(env, buf);
>> }
>> x = JNU_NewObjectByName(env,
>> "java/io/FileNotFoundException",
>> "(Ljava/lang/String;Ljava/lang/String;)V",
>> path, why);
>> if (x != NULL) {
>> - (*env)->Throw(env, x);
>> + (*env)->Throw(env, x);
>> }
>> }
>> diff --git a/src/share/native/java/io/io_util.h
>> b/src/share/native/java/io/io_util.h
>> --- a/src/share/native/java/io/io_util.h
>> +++ b/src/share/native/java/io/io_util.h
>> @@ -38,9 +38,9 @@ extern jfieldID IO_handle_fdID;
>> * IO helper functions
>> */
>>
>> -int readSingle(JNIEnv *env, jobject this, jfieldID fid);
>> -int readBytes(JNIEnv *env, jobject this, jbyteArray bytes, jint off,
>> - jint len, jfieldID fid);
>> +jint readSingle(JNIEnv *env, jobject this, jfieldID fid);
>> +jint readBytes(JNIEnv *env, jobject this, jbyteArray bytes, jint off,
>> + jint len, jfieldID fid);
>> void writeSingle(JNIEnv *env, jobject this, jint byte, jfieldID fid);
>> void writeBytes(JNIEnv *env, jobject this, jbyteArray bytes, jint off,
>> jint len, jfieldID fid);
>>
>> Martin
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SignedIntegerOverflow.patch
Type: text/x-patch
Size: 6963 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20090106/473a59f8/SignedIntegerOverflow.patch>
More information about the core-libs-dev
mailing list