[PATCH] 6788196: Array bounds checks in io_util.c rely on undefined behaviour
Martin Buchholz
martinrb at google.com
Wed Dec 24 22:41:16 UTC 2008
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
On Wed, Dec 24, 2008 at 02:25, Gary Benson <gbenson at redhat.com> wrote:
> Martin Buchholz wrote:
>> Does this actually change the behavior with recent gccs?
>
> I don't think anything changed recently, not on Intel or SPARC,
> but I develop on PowerPC, and GCC on 32-bit PowerPC seems to
> overflow to 1, -1 or 0... sometimes. But that's not the point;
> the behaviour is undefined, so the result could be anything,
> on any platform.
>
>> It seems like the introduction of uint32_t is trading one
>> non-portability for another, namely relying on C99 features.
>> I have been waiting patiently for C99 compilers to emerge,
>> but gcc for example is still not there yet.
>> http://gcc.gnu.org/c99status.html
>>
>> If you are going to use types like uint32_t, you should
>> be including the standard header that defines them - <stdint.h>
>
> I didn't realise there was a header required. I remembered I'd
> seen something similar in HotSpot so I just copied that piece of
> code.
>
>> More immediate and obvious improvements to the code would be to
>> change the type of datalen to "jsize" and the type of nread to
>> "jint".
>>
>> I suggest, instead of using unsigned types, is to do what
>> java code would do in a case like this, and cast to jlong
>> instead of uint32_t to avoid overflow. I approve this patch
>> if you make that change.
>
> Would it not simply be better to cast to unsigned int? I don't
> know about other platforms, but on 32-bit PowerPC casting to
> jlong would use three more registers and add a load of extra
> instructions whereas casting to unsigned int adds none. The
> impact on performance and memory usage would be negligable of
> course, but using 64-bit types where I don't need to makes me
> a little uneasy...
>
>> I see you've eliminated one of the checks, which was unnecessary.
>> Thanks for that.
>
> No problem :)
>
> Cheers,
> Gary
>
> --
> http://gbenson.net/
>
More information about the core-libs-dev
mailing list