This is the mail archive of the
java-patches@gcc.gnu.org
mailing list for the Java project.
Re: Another wrong array index in natClass.cc
- To: martin dot kahlert at infineon dot com
- Subject: Re: Another wrong array index in natClass.cc
- From: Bryce McKinlay <bryce at waitaki dot otago dot ac dot nz>
- Date: Tue, 05 Jun 2001 21:51:24 +1200
- CC: java-patches at gcc dot gnu dot org
- References: <20010521114739.A20244@keksy.muc.infineon.com>
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