This is the mail archive of the java-patches@gcc.gnu.org mailing list for the Java project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Patch] natString.cc: Some fixes for bounds checking arithmetic.


libgcj contains many places where checking of array index arguments
passed to methods is done incorrectly.

The attached patch fixes those in natString.cc that it is not difficult
to write test cases for.

One of the test cases in the patch is commented out - it causes a
warning during the link, which seems to cause the test to fail
extraneously.  If someone can tell me how to turn off or ignore the
warning message, I will do so and uncomment the test case.

This patch passes a make check in libjava.

There are many other instances of incorrect bounds checking that would
require a 64 bit machine with gigabytes of memory (which I don't have),
in order to write proper test cases.

Alternatively, I could fix those without test cases.  Would that be
acceptable?

Ralph.

2003-09-23  Ralph Loader  <suckfish@ihug.co.nz>

	* java/lang/natString.cc (getChars):
	Fix validation of array indexes.
	(getBytes, regionMatches, startsWith, valueOf): Likewise.
	* testsuite/libjava.lang/String_overflow.java: New file.
 	* testsuite/libjava.lang/String_overflow.out: New file.


Index: libjava/java/lang/natString.cc
===================================================================
RCS file: /cvsroot/gcc/gcc/libjava/java/lang/natString.cc,v
retrieving revision 1.31
diff -u -u -r1.31 natString.cc
--- libjava/java/lang/natString.cc	28 Jul 2003 16:12:00 -0000	1.31
+++ libjava/java/lang/natString.cc	23 Sep 2003 06:16:12 -0000
@@ -590,7 +590,10 @@
   jint dst_length = JvGetArrayLength (dst);
   if (srcBegin < 0 || srcBegin > srcEnd || srcEnd > count)
     throw new java::lang::StringIndexOutOfBoundsException;
-  if (dstBegin < 0 || dstBegin + (srcEnd-srcBegin) > dst_length)
+  // The 2nd part of the test below is equivalent to 
+  // dstBegin + (srcEnd-srcBegin) > dst_length
+  // except that it does not overflow.
+  if (dstBegin < 0 || dstBegin > dst_length - (srcEnd-srcBegin))
     throw new ArrayIndexOutOfBoundsException;
   jchar *dPtr = elements (dst) + dstBegin;
   jchar *sPtr = JvGetStringChars (this) + srcBegin;
@@ -642,7 +645,10 @@
   jint dst_length = JvGetArrayLength (dst);
   if (srcBegin < 0 || srcBegin > srcEnd || srcEnd > count)
     throw new java::lang::StringIndexOutOfBoundsException;
-  if (dstBegin < 0 || dstBegin + (srcEnd-srcBegin) > dst_length)
+  // The 2nd part of the test below is equivalent to 
+  // dstBegin + (srcEnd-srcBegin) > dst_length
+  // except that it does not overflow.
+  if (dstBegin < 0 || dstBegin > dst_length - (srcEnd-srcBegin))
     throw new ArrayIndexOutOfBoundsException;
   jbyte *dPtr = elements (dst) + dstBegin;
   jchar *sPtr = JvGetStringChars (this) + srcBegin;
@@ -689,9 +695,9 @@
 java::lang::String::regionMatches (jint toffset,
 				   jstring other, jint ooffset, jint len)
 {
-  if (toffset < 0 || ooffset < 0
-      || toffset + len > count
-      || ooffset + len > other->count)
+  if (toffset < 0 || ooffset < 0 || len < 0
+      || toffset > count - len
+      || ooffset > other->count - len)
     return false;
   jchar *tptr = JvGetStringChars (this) + toffset;
   jchar *optr = JvGetStringChars (other) + ooffset;
@@ -726,9 +732,9 @@
 java::lang::String::regionMatches (jboolean ignoreCase, jint toffset,
 				   jstring other, jint ooffset, jint len)
 {
-  if (toffset < 0 || ooffset < 0
-      || toffset + len > count
-      || ooffset + len > other->count)
+  if (toffset < 0 || ooffset < 0 || len < 0
+      || toffset > count - len
+      || ooffset > other->count - len)
     return false;
   jchar *tptr = JvGetStringChars (this) + toffset;
   jchar *optr = JvGetStringChars (other) + ooffset;
@@ -759,7 +765,7 @@
 java::lang::String::startsWith (jstring prefix, jint toffset)
 {
   jint i = prefix->count;
-  if (toffset < 0 || toffset + i > count)
+  if (toffset < 0 || toffset > count - i)
     return false;
   jchar *xptr = JvGetStringChars (this) + toffset;
   jchar *yptr = JvGetStringChars (prefix);
@@ -1032,7 +1038,7 @@
 java::lang::String::valueOf(jcharArray data, jint offset, jint count)
 {
   jint data_length = JvGetArrayLength (data);
-  if (offset < 0 || count < 0 || offset+count > data_length)
+  if (offset < 0 || count < 0 || offset > data_length - count)
     throw new ArrayIndexOutOfBoundsException;
   jstring result = JvAllocString(count);
   jchar *sPtr = elements (data) + offset;
--- /dev/null	2003-09-16 01:40:47.000000000 +1200
+++ libjava/testsuite/libjava.lang/String_overflow.java	2003-09-23 18:09:00.737170037 +1200
@@ -0,0 +1,140 @@
+class String_overflow
+{
+  static void getChars()
+  {
+    String source = "abcdefg";
+    char[] dest = new char [3];
+
+    try
+      {
+	source.getChars (0, 5,	// Source
+			 dest, (1<<31) - 1);
+	Fail ("getChars", "Should not have succeeded");
+      }
+    catch (Throwable e)
+      {
+	ExpectArrayIndex ("getChars", e);
+      }
+  }
+
+    /* How do I stop a compiler warning causing a test to fail?
+  static void getBytes()
+  {
+    String source = "abcdefg";
+    byte[] dest = new byte[3];
+
+    try
+      {
+	source.getBytes (0, 5, dest, (1<<31) - 1);
+	Fail ("getBytes", "Should not have succeeded");
+      }
+    catch (Throwable e)
+      {
+	ExpectArrayIndex ("getBytes", e);
+      }
+  }
+    */
+
+  static void regionMatches()
+  {
+    if ("abcdefg".regionMatches (4, "abcdefg", 4, -1))
+      {
+	Fail ("regionMatches", "Should not return true");
+      }
+
+    try
+      {
+	if ("abcdefg".regionMatches (4, "abcdefg", 4, (1<<31)-1))
+	  {
+	    Fail ("regionMatches (2nd)", "Should not return true");
+	  }
+      }
+    catch (Throwable e)
+      {
+	Fail ("regionMatches (2nd)", e);
+      }
+  }
+
+  static void regionMatchesCase()
+  {
+    if ("abcdefg".regionMatches (true, 4, "abcdefg", 4, -1))
+      {
+	Fail ("regionMatchesCase", "Should not return true");
+      }
+
+    try
+      {
+	if ("abcdefg".regionMatches (true, 4, "abcdefg", 4, (1<<31)-1))
+	  {
+	    Fail ("regionMatchesCase (2nd)", "Should not return true");
+	  }
+      }
+    catch (Throwable e)
+      {
+	Fail ("regionMatchesCase (2nd)", e);
+      }
+  }
+
+  static void startsWith()
+  {
+    // We make the arg pretty big to try and cause a segfault.
+    String s = new String ("abcdef");
+    StringBuffer b = new StringBuffer (1000000);
+    b.setLength (1000000);
+    String arg = new String (b);
+
+    try
+      {
+	s.startsWith (arg, (1<<31) - 1000000);
+      }
+    catch (Throwable e)
+      {
+	Fail ("startsWith", e);
+      }
+  }
+
+  static void valueOf()
+  {
+    char[] array = new char[] {'a', 'b', 'c', 'd', 'e'};
+    try
+      {
+	String.valueOf (array, 4, (1<<31)-1);
+	Fail ("valueOf", "should not succeed");
+      }
+    catch (Throwable e)
+      {
+	ExpectArrayIndex ("valueOf", e);
+      }
+  }
+
+  public static void main (String[] args) throws Throwable
+  {
+    getChars();
+    //    getBytes();
+    regionMatches();
+    regionMatchesCase();
+    startsWith();
+    valueOf();
+
+    if (tests_failed == 0)
+      System.out.println ("ok");
+  }
+
+  static void ExpectArrayIndex (String test, Throwable e)
+  {
+    if (e instanceof ArrayIndexOutOfBoundsException)
+      return;
+
+    Fail (test, e);
+  }
+
+  static void Fail (String test, Object problem)
+  {
+    ++tests_failed;
+    System.err.print (test);
+    System.err.print ('\t');
+    System.err.println (problem);
+  }
+
+  static int tests_failed;
+}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]