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
diff -r 201a75db9b35 openjdk/jdk/src/share/native/java/io/io_util.c
--- openjdk/jdk/src/share/native/java/io/io_util.c Mon Dec 22 12:32:44
2008 +0000
+++ openjdk/jdk/src/share/native/java/io/io_util.c Mon Dec 22 12:48:49
2008 +0000
@@ -25,6 +25,7 @@
#include <stdlib.h>
#include <string.h>
+#include <assert.h>
#include "jni.h"
#include "jni_util.h"
@@ -73,9 +74,13 @@
return -1;
}
datalen = (*env)->GetArrayLength(env, bytes);
+ assert(datalen >= 0);
- if ((off < 0) || (off > datalen) ||
- (len < 0) || ((off + len) > datalen) || ((off + len) < 0)) {
+ // If you modify this check be very careful to avoid signed
+ // integer overflow, the result of which is undefined in C.
+ // For more details see bug 6788196, and the thread starting at
+ //
http://mail.openjdk.java.net/pipermail/core-libs-dev/2008-December/000955.html
+ if ((off < 0) || (len < 0) || (datalen - off < len)) {
JNU_ThrowByName(env, "java/lang/IndexOutOfBoundsException", 0);
return -1;
}
@@ -146,9 +151,13 @@
return;
}
datalen = (*env)->GetArrayLength(env, bytes);
+ assert(datalen >= 0);
- if ((off < 0) || (off > datalen) ||
- (len < 0) || ((off + len) > datalen) || ((off + len) < 0)) {
+ // If you modify this check be very careful to avoid signed
+ // integer overflow, the result of which is undefined in C.
+ // For more details see bug 6788196, and the thread starting at
+ //
http://mail.openjdk.java.net/pipermail/core-libs-dev/2008-December/000955.html
+ if ((off < 0) || (len < 0) || (datalen - off < len)) {
JNU_ThrowByName(env, "java/lang/IndexOutOfBoundsException", 0);
return;
}
