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 <[email protected]> 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
>
6788196: (porting) Bounds checks in io_util.c rely on undefined behaviour
Reviewed-By: alanb
Contributed-By: [email protected]
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
@@ -58,12 +58,24 @@
*/
#define BUF_SIZE 8192
+/*
+ * Returns true if the array slice defined by the given offset and length
+ * is out of bounds.
+ */
+static int
+outOfBounds(JNIEnv *env, jint off, jint len, jbyteArray array) {
+ return ((off < 0) ||
+ (len < 0) ||
+ // We are very careful to avoid signed integer overflow,
+ // the result of which is undefined in C.
+ ((*env)->GetArrayLength(env, array) - off < len));
+}
int
readBytes(JNIEnv *env, jobject this, jbyteArray bytes,
jint off, jint len, jfieldID fid)
{
- int nread, datalen;
+ int nread;
char stackBuf[BUF_SIZE];
char *buf = 0;
FD fd;
@@ -72,10 +84,8 @@
JNU_ThrowNullPointerException(env, 0);
return -1;
}
- datalen = (*env)->GetArrayLength(env, bytes);
- if ((off < 0) || (off > datalen) ||
- (len < 0) || ((off + len) > datalen) || ((off + len) < 0)) {
+ if (outOfBounds(env, off, len, bytes)) {
JNU_ThrowByName(env, "java/lang/IndexOutOfBoundsException", 0);
return -1;
}
@@ -136,7 +146,7 @@
writeBytes(JNIEnv *env, jobject this, jbyteArray bytes,
jint off, jint len, jfieldID fid)
{
- int n, datalen;
+ int n;
char stackBuf[BUF_SIZE];
char *buf = 0;
FD fd;
@@ -145,10 +155,8 @@
JNU_ThrowNullPointerException(env, 0);
return;
}
- datalen = (*env)->GetArrayLength(env, bytes);
- if ((off < 0) || (off > datalen) ||
- (len < 0) || ((off + len) > datalen) || ((off + len) < 0)) {
+ if (outOfBounds(env, off, len, bytes)) {
JNU_ThrowByName(env, "java/lang/IndexOutOfBoundsException", 0);
return;
}
diff --git a/test/java/io/readBytes/ReadBytesBounds.java b/test/java/io/readBytes/ReadBytesBounds.java
--- a/test/java/io/readBytes/ReadBytesBounds.java
+++ b/test/java/io/readBytes/ReadBytesBounds.java
@@ -23,7 +23,7 @@
/*
@test
- @bug 4017728 4079849
+ @bug 4017728 4079849 6788196
@summary Check for correct Array Bounds check in read of FileInputStream and
RandomAccessFile
*/
@@ -42,87 +42,57 @@
public class ReadBytesBounds {
- public static void main(String argv[]) throws Exception{
+ static final FileInputStream fis;
+ static final RandomAccessFile raf;
+ static final byte[] b = new byte[32];
- int num_test_cases = 12;
- int off[] = {-1 , -1 , 0 , 0 , 33 , 33 , 0 , 32 , 32 , 4 , 1 , 0};
- int len[] = {-1 , 0 , -1 , 33 , 0 , 4 , 32 , 0 , 4 , 16 , 31 , 0};
- boolean results[] = { false , false , false , false , false , false ,
- true , true , false , true , true , true};
+ 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);
+ }
- FileInputStream fis = null;
- RandomAccessFile raf = null;
- byte b[] = new byte[32];
-
- int num_good = 0;
- int num_bad = 0;
-
- String dir = System.getProperty("test.src", ".");
- File testFile = new File(dir, "input.txt");
- fis = new FileInputStream(testFile);
- for(int i = 0; i < num_test_cases; i++) {
-
- try {
- int bytes_read = fis.read(b , off[i] , len[i]);
- } catch(IndexOutOfBoundsException aiobe) {
- if (results[i]) {
- throw new RuntimeException("Unexpected result");
- }
- else {
- num_good++;
- }
- continue;
- }
-
- if (results[i]) {
- num_good++;
- }
- else {
- throw new RuntimeException("Unexpected result");
- }
-
- }
- System.out.println("Results for FileInputStream.read");
- System.out.println("\nTotal number of test cases = " + num_test_cases +
- "\nNumber succeded = " + num_good +
- "\nNumber failed = " + num_bad);
-
-
-
- num_good = 0;
- num_bad = 0;
-
- raf = new RandomAccessFile(testFile , "r");
- for(int i = 0; i < num_test_cases; i++) {
-
- try {
- int bytes_read = raf.read(b , off[i] , len[i]);
- } catch(IndexOutOfBoundsException aiobe) {
- if (results[i]) {
- throw new RuntimeException("Unexpected result");
- }
- else {
- num_good++;
- }
- continue;
- }
-
- if (results[i]) {
- num_good++;
- }
- else {
- throw new RuntimeException("Unexpected result");
- }
-
+ 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;
}
- System.out.println("Results for RandomAccessFile.read");
- System.out.println("\nTotal number of test cases = " + num_test_cases +
- "\nNumber succeded = " + num_good +
- "\nNumber failed = " + num_bad);
-
-
+ if (result != expected) {
+ throw new RuntimeException
+ (String.format("Unexpected result off=%d len=%d expected=%b",
+ off, len, expected));
+ }
}
-
}
