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]

Re: Another wrong array index in natClass.cc


Martin Kahlert wrote:

> This needs someone with better knowledge of natClass.cc than me:
> The patch fixes the obvious bug, but the problem may be deeper.
> In my application offset was -1 (found by Electric Fence and EF_PROTECT_BELOW)

A value of -1 in the ioffsets table indicates an unused slot (see _Jv_GenerateITable). This means that
the source class does not implement the target interface, so the result is false. Now, the original
idea here is that class->itable could be generated at compile time. I'm not sure if we want to do that
now given code bloat concerns, but that way we'd be able to ensure that accessing the -1 position
won't segfault and can never be an interface pointer. Currently, of course, the itable is allocated
with malloc() so accessing -1 could possibly segfault or even return a false positive if the value
there just happened to point to the right interface, so this is an important fix. However, I'd like to
explicitly check for the -1 value (rather than > 0) to make this clearer.

> *** 947,952 ****
> --- 947,953 ----
>
>     if ((target == &ObjectClass && !source->isPrimitive())
>         || (source->ancestors != NULL
> +         && source->depth >= target->depth && target->depth > 0
>           && source->ancestors[source->depth - target->depth] == target))
>       return true;
>
> By printing the index in my application it found out, that it
> sometimes was -4. Thus the first check.
>

Right - we are not checking for the case where the target has a greater depth than the source, which
is bad for the same reasons as above: possibility of segfault or false positive. Nice catch.

> The second one may be neccessary if target is a java.lang.Object (depth == 0)
> and source->isPrimitive() == true (--> can this happen?).
>

I think its better to just rearrange things slightly here so that the second branch wont be taken in
the source->isPrimitive() case. The only other way target->depth could be <= 0 is if
target->isPrimitive (depth -1). We need to add an explicit test for this case.

I'm going to check in the modified patch below to both mainline and the branch. Thanks much for
tracking these down, and I apologize for the long delay in reviewing the patch.

regards

  [ bryce ]


2001-05-31  Martin Kahlert  <martin.kahlert@infineon.com>
	    Bryce McKinlay  <bryce@waitaki.otago.ac.nz>

	* java/lang/natClass.cc (_Jv_IsAssignableFrom): Ensure that ancestors
	table index is within allowed bounds. Ensure that we dont try to access
	class itable at a negative offset. Avoid an ancestor table lookup if
	source is a primitive type class.
	(isInstance): Remove redundant isPrimitive() check.

Index: natClass.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natClass.cc,v
retrieving revision 1.43
diff -u -r1.43 natClass.cc
--- natClass.cc	2001/05/18 06:29:11	1.43
+++ natClass.cc	2001/06/05 09:08:00
@@ -633,7 +633,7 @@
 jboolean
 java::lang::Class::isInstance (jobject obj)
 {
-  if (__builtin_expect (! obj || isPrimitive (), false))
+  if (! obj)
     return false;
   _Jv_InitClass (this);
   return _Jv_IsAssignableFrom (this, JV_CLASS (obj));
@@ -939,19 +939,29 @@
       if (cl_iindex < if_idt->iface.ioffsets[0])
         {
 	  jshort offset = if_idt->iface.ioffsets[cl_iindex];
-	  if (offset < cl_idt->cls.itable_length
+	  if (offset != -1 && offset < cl_idt->cls.itable_length
 	      && cl_idt->cls.itable[offset] == target)
 	    return true;
 	}
       return false;
     }
      
-  if ((target == &ObjectClass && !source->isPrimitive())
-      || (source->ancestors != NULL 
-	  && source->ancestors[source->depth - target->depth] == target))
+  // Primitive TYPE classes are only assignable to themselves.
+  if (__builtin_expect (target->isPrimitive(), false))
+    return false;
+    
+  if (target == &ObjectClass)
+    {
+      if (source->isPrimitive())
+        return false;
+      return true;
+    }
+  else if (source->ancestors != NULL 
+           && source->depth >= target->depth
+	   && source->ancestors[source->depth - target->depth] == target)
     return true;
       
- return false;
+  return false;
 }
 
 // Interface type checking, the slow way. Returns TRUE if IFACE is a 

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