[PATCH] 6788196: Array bounds checks in io_util.c rely on undefined behaviour
Hi all,
In C, the result of an overflowing add of two signed integers is
undefined. The array bounds checks in readBytes and writeBytes
in jdk/src/share/native/java/io/io_util.c, however, rely on the
assumption that the result of the overflowing add will be negative.
The attached patch fixes.
Cheers,
Gary
--
http://gbenson.net/
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 +
+++ openjdk/jdk/src/share/native/java/io/io_util.c Mon Dec 22 12:48:49
2008 +
@@ -25,6 +25,7 @@
#include stdlib.h
#include string.h
+#include assert.h
#include jni.h
#include jni_util.h
@@ -73,9 +74,10 @@
return -1;
}
datalen = (*env)-GetArrayLength(env, bytes);
+assert(datalen = 0);
-if ((off 0) || (off datalen) ||
-(len 0) || ((off + len) datalen) || ((off + len) 0)) {
+if ((off 0) || (len 0) ||
+(((uint32_t) off + (uint32_t) len) (uint32_t) datalen)) {
JNU_ThrowByName(env, java/lang/IndexOutOfBoundsException, 0);
return -1;
}
@@ -146,9 +148,10 @@
return;
}
datalen = (*env)-GetArrayLength(env, bytes);
+assert(datalen = 0);
-if ((off 0) || (off datalen) ||
-(len 0) || ((off + len) datalen) || ((off + len) 0)) {
+if ((off 0) || (len 0) ||
+(((uint32_t) off + (uint32_t) len) (uint32_t) datalen)) {
JNU_ThrowByName(env, java/lang/IndexOutOfBoundsException, 0);
return;
}
/* @test
* @bug 6788196
* @summary Check that file IO array bounds checks are applied
*/
import java.io.File;
import java.io.FileInputStream;
public class Test6788196 {
public static void main(String[] args) throws Exception {
File file = new File(
System.getProperty(test.src, .), Test6779290.java);
FileInputStream fis = new FileInputStream(file);
byte[] b = new byte[20];
try {
fis.read(b, 10, Integer.MAX_VALUE);
}
catch (ArrayIndexOutOfBoundsException e) {
throw e;
}
catch (IndexOutOfBoundsException e) {
}
}
}
hg: jdk7/tl/jdk: 6787009: (attach) Stub injection potentially unsafe on windows-x64
Changeset: 3d09cc6c4ea9 Author:alanb Date: 2008-12-22 19:28 + URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/3d09cc6c4ea9 6787009: (attach) Stub injection potentially unsafe on windows-x64 Reviewed-by: mchung ! src/windows/native/sun/tools/attach/WindowsVirtualMachine.c
Re: [PATCH] 6788196: Array bounds checks in io_util.c rely on undefined behaviour
Hi Gary, Does this actually change the behavior with recent gccs? 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 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. I see you've eliminated one of the checks, which was unnecessary. Thanks for that. Martin On Tue, Dec 23, 2008 at 02:21, Gary Benson gben...@redhat.com wrote: Hi all, In C, the result of an overflowing add of two signed integers is undefined. The array bounds checks in readBytes and writeBytes in jdk/src/share/native/java/io/io_util.c, however, rely on the assumption that the result of the overflowing add will be negative. The attached patch fixes. Cheers, Gary -- http://gbenson.net/
Re: [PATCH] 6788196: Array bounds checks in io_util.c rely on undefined behaviour
In C, the result of an overflowing add of two signed integers is undefined. Strewth! That's a surprise to me. I always thought that C defined integer arithmetic to always wrap. Checking for a negative to detect overflow is a common pattern - heck it's THE pattern for detecting overflow according to Hacker's Delight! But it isn't valid C ! ??? Even in Java this pattern is used a lot - I guess it's lucky the VM goes straight to machine code not C otherwise we'd be in trouble! David Holmes Martin Buchholz said the following on 12/24/08 06:42: Hi Gary, Does this actually change the behavior with recent gccs? 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 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. I see you've eliminated one of the checks, which was unnecessary. Thanks for that. Martin On Tue, Dec 23, 2008 at 02:21, Gary Benson gben...@redhat.com wrote: Hi all, In C, the result of an overflowing add of two signed integers is undefined. The array bounds checks in readBytes and writeBytes in jdk/src/share/native/java/io/io_util.c, however, rely on the assumption that the result of the overflowing add will be negative. The attached patch fixes. Cheers, Gary -- http://gbenson.net/
Re: [PATCH] 6788196: Array bounds checks in io_util.c rely on undefined behaviour
David Holmes - Sun Microsystems wrote: In C, the result of an overflowing add of two signed integers is undefined. Strewth! That's a surprise to me. I always thought that C defined integer arithmetic to always wrap. Only for unsigned operands (from 6.2.5 - Types): A computation involving unsigned operands can never overflow, because a result that cannot be represented by the resulting unsigned integer type is reduced modulo the number that is one greater than the largest value modulo reduced that can be represented by the resulting type. see p. 496 in the The New C Standard: An Economic and Cultural Commentary for more details then most people care. ;) cheers, dalibor topic -- *** Dalibor Topic Tel: (+49 40) 23 646 738 Java F/OSS Ambassador AIM: robiladonaim Sun Microsystems GmbH Mobile: (+49 177) 2664 192 Nagelsweg 55http://openjdk.java.net D-20097 Hamburg mailto:dalibor.to...@sun.com Sitz der Gesellschaft: Sonnenallee 1, D-85551 Kirchheim-Heimstetten Amtsgericht München: HRB 161028 Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer Vorsitzender des Aufsichtsrates: Martin Häring
