problem with class accessiblity check in invoke (natMethod.cc)

problem with class accessiblity check in invoke (natMethod.cc)

[Erik Groeneveld]

L.S.

We recently upgraded from libgcj7 to libgcj9. This broke our code. We
found out that in natMethod.cc in invoke() there is an additional
check for class accessibility. It starts out with:

  // Check accessibility, if required.
  if (! this->isAccessible())
    {
      if (! (Modifier::isPublic (meth->accflags)))
	{
	  Class *caller = _Jv_StackTrace::GetCallingClass (&Method::class$);
	  if (! _Jv_CheckAccess(caller, declaringClass, meth->accflags))
	    throw new IllegalAccessException;
	}

and then continues here (this part is new):

      else
	// Method is public, check to see if class is accessible.
	{
	  jint flags = (declaringClass->accflags
			& (Modifier::PUBLIC
			   | Modifier::PROTECTED
			   | Modifier::PRIVATE));
	  if (flags == 0) // i.e. class is package private
	    {
	      Class *caller = _Jv_StackTrace::GetCallingClass (&Method::class$);
	      if (! _Jv_ClassNameSamePackage (caller->name,
					      declaringClass->name))
		throw new IllegalAccessException;
	    }
	}
    }

The added code prevents legal calls to fail. For example, it is no
longer possible to call invoke on the ´hasNext´ method on the iterator
from AbstractList.  The method AbstractList.iterator() returns an
anonymous innerclass object which according to the C code above is not
accessible since it is package private.  There are two problems here:

1. The systems segfaults on the caller->name because there is no calling class.
2. We believe class accessibility is not relevant here: there is no
reason why a Method object with public access should not be invokable,
or is there?

We found the following modification log, which is why we address
Andrew Haley directly as well as the list:

2007-04-16  Andrew Haley  <aph@redhat.com>

        * java/lang/reflect/natMethod.cc (Method::invoke): In invoke also
        check that the method's declaring class is accessible.

Can you or anybody else explain why this was considered to be necessary?

Yours,
Erik J. Groeneveld and Jurjan-Paul Medema

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Andrew Haley]

On 12/23/2009 10:38 AM, Erik Groeneveld wrote:

> We recently upgraded from libgcj7 to libgcj9. This broke our code. We
> found out that in natMethod.cc in invoke() there is an additional
> check for class accessibility. It starts out with:
> 
>   // Check accessibility, if required.
>   if (! this->isAccessible())
>     {
>       if (! (Modifier::isPublic (meth->accflags)))
> 	{
> 	  Class *caller = _Jv_StackTrace::GetCallingClass (&Method::class$);
> 	  if (! _Jv_CheckAccess(caller, declaringClass, meth->accflags))
> 	    throw new IllegalAccessException;
> 	}
> 
> and then continues here (this part is new):
> 
>       else
> 	// Method is public, check to see if class is accessible.
> 	{
> 	  jint flags = (declaringClass->accflags
> 			& (Modifier::PUBLIC
> 			   | Modifier::PROTECTED
> 			   | Modifier::PRIVATE));
> 	  if (flags == 0) // i.e. class is package private
> 	    {
> 	      Class *caller = _Jv_StackTrace::GetCallingClass (&Method::class$);
> 	      if (! _Jv_ClassNameSamePackage (caller->name,
> 					      declaringClass->name))
> 		throw new IllegalAccessException;
> 	    }
> 	}
>     }
> 
> The added code prevents legal calls to fail. For example, it is no
> longer possible to call invoke on the ´hasNext´ method on the iterator
> from AbstractList.  The method AbstractList.iterator() returns an
> anonymous innerclass object which according to the C code above is not
> accessible since it is package private.  There are two problems here:
> 
> 1. The systems segfaults on the caller->name because there is no calling class.

How can there be no calling class?

> 2. We believe class accessibility is not relevant here: there is no
> reason why a Method object with public access should not be invokable,
> or is there?
> 
> We found the following modification log, which is why we address
> Andrew Haley directly as well as the list:
> 
> 2007-04-16  Andrew Haley  <aph@redhat.com>
> 
>         * java/lang/reflect/natMethod.cc (Method::invoke): In invoke also
>         check that the method's declaring class is accessible.
> 
> Can you or anybody else explain why this was considered to be necessary?

Yes, but can you send a test case before we go any further?  Then at least
we'll all know what you're talking about.

Andrew.

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Erik Groeneveld]

Hello Andrew,

Thanks for your quick reply. We include code below.

On Wed, Dec 23, 2009 at 11:46, Andrew Haley <aph@redhat.com> wrote:
> On 12/23/2009 10:38 AM, Erik Groeneveld wrote:
>
[...]
>>       else
>>       // Method is public, check to see if class is accessible.
>>       {
>>         jint flags = (declaringClass->accflags
>>                       & (Modifier::PUBLIC
>>                          | Modifier::PROTECTED
>>                          | Modifier::PRIVATE));
>>         if (flags == 0) // i.e. class is package private
>>           {
>>             Class *caller = _Jv_StackTrace::GetCallingClass (&Method::class$);
>>             if (! _Jv_ClassNameSamePackage (caller->name,
>>                                             declaringClass->name))
>>               throw new IllegalAccessException;
>>           }
>>       }
>>     }
>>
[...]

>> 1. The systems segfaults on the caller->name because there is no calling class.
>
> How can there be no calling class?

Because we call it from C++ not from Java.  We forgot to mention that.

>> 2. We believe class accessibility is not relevant here: there is no
>> reason why a Method object with public access should not be invokable,
>> or is there?
[...]
>
> Yes, but can you send a test case before we go any further?  Then at least
> we'll all know what you're talking about.

The following code demonstrates the problem.

#include <gcj/cni.h>
#include <java/util/ArrayList.h>
#include <java/util/Iterator.h>
#include <java/lang/reflect/Method.h>
#include <stdio.h>

int main(int argc, char* argv[]) {
    JvCreateJavaVM(NULL);
    JvAttachCurrentThread(NULL, NULL);
    java::util::ArrayList* l = new java::util::ArrayList();
    java::util::Iterator* i = l->iterator();
    java::lang::reflect::Method* m = i->getClass()->getDeclaredMethod(
            JvNewStringUTF("hasNext"), NULL);
    printf("calling invoke, it'll dump core in natMethod.cc line 194\n");
    m->invoke(i, NULL);
    return 0;
}

$gcc problem.cpp -lgcj
$./a.out
calling invoke
Aborted (core dumped)

$gdb -core core a.out
(gdb) where
#0  0x00002adf9252bed5 in raise () from /lib/libc.so.6
#1  0x00002adf9252d3f3 in abort () from /lib/libc.so.6
#2  0x00002adf90bdeed8 in _Jv_Throw (value=0x2adf932cd370) at
../../../src/libjava/exception.cc:128
#3  0x00002adf90bd2a2a in _Jv_catch_segv (_p=<value optimized out>) at
../../../src/libjava/prims.cc:184
#4  <signal handler called>
#5  0x00002adf90c217d3 in java::lang::reflect::Method::invoke
(this=0x2adf932d1c80, obj=0x2adf93ba6e40,
    args=0x0) at ../../../src/libjava/java/lang/reflect/natMethod.cc:194
#6  0x0000000000400a5c in main ()

The top of the stack is from the NULL-pointer catching signal handler
we believe, so #5 and #6 are the relevant ones.

The point is that we believe that the scenario in the C++ code is
valid, both from Java and from C++, and we do not see the reasons for
the additional check that has been added to the invoke() method.

Erik
Jurjan-Paul

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Bryce McKinlay]

On Wed, Dec 23, 2009 at 1:34 PM, Erik Groeneveld <erik@cq2.nl> wrote:

> The following code demonstrates the problem.
>
> #include <gcj/cni.h>
> #include <java/util/ArrayList.h>
> #include <java/util/Iterator.h>
> #include <java/lang/reflect/Method.h>
> #include <stdio.h>
>
> int main(int argc, char* argv[]) {
>    JvCreateJavaVM(NULL);
>    JvAttachCurrentThread(NULL, NULL);
>    java::util::ArrayList* l = new java::util::ArrayList();
>    java::util::Iterator* i = l->iterator();
>    java::lang::reflect::Method* m = i->getClass()->getDeclaredMethod(
>            JvNewStringUTF("hasNext"), NULL);
>    printf("calling invoke, it'll dump core in natMethod.cc line 194\n");
>    m->invoke(i, NULL);
>    return 0;
> }
>
> $gcc problem.cpp -lgcj
> $./a.out
> calling invoke
> Aborted (core dumped)
>
> $gdb -core core a.out
> (gdb) where
> #0  0x00002adf9252bed5 in raise () from /lib/libc.so.6
> #1  0x00002adf9252d3f3 in abort () from /lib/libc.so.6
> #2  0x00002adf90bdeed8 in _Jv_Throw (value=0x2adf932cd370) at
> ../../../src/libjava/exception.cc:128
> #3  0x00002adf90bd2a2a in _Jv_catch_segv (_p=<value optimized out>) at
> ../../../src/libjava/prims.cc:184
> #4  <signal handler called>
> #5  0x00002adf90c217d3 in java::lang::reflect::Method::invoke
> (this=0x2adf932d1c80, obj=0x2adf93ba6e40,
>    args=0x0) at ../../../src/libjava/java/lang/reflect/natMethod.cc:194
> #6  0x0000000000400a5c in main ()
>
> The top of the stack is from the NULL-pointer catching signal handler
> we believe, so #5 and #6 are the relevant ones.
>
> The point is that we believe that the scenario in the C++ code is
> valid, both from Java and from C++, and we do not see the reasons for
> the additional check that has been added to the invoke() method.

Method.invoke() is required to check accessibility according to the
Java Language Specification, so this check is required.

The only thing libgcj is doing wrong is failing to check for null
"caller" (which can't happen in Java code). It should probably do this
check and throw an IllegalAccessException, or maybe just permit the
access.

A workaround is to just add a Method.setAccessible() call to your code
to bypass access checks.

Bryce

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Erik Groeneveld]

(Sorry for the HTML post)

On 23 dec 2009, at 16:09, Bryce McKinlay <bmckinlay@gmail.com> wrote:
> On Wed, Dec 23, 2009 at 1:34 PM, Erik Groeneveld <erik@cq2.nl> wrote:
>
>> The following code demonstrates the problem.
>>
>> #include <gcj/cni.h>
>> #include <java/util/ArrayList.h>
>> #include <java/util/Iterator.h>
>> #include <java/lang/reflect/Method.h>
>> #include <stdio.h>
>>
>> int main(int argc, char* argv[]) {
>>    JvCreateJavaVM(NULL);
>>    JvAttachCurrentThread(NULL, NULL);
>>    java::util::ArrayList* l = new java::util::ArrayList();
>>    java::util::Iterator* i = l->iterator();
>>    java::lang::reflect::Method* m = i->getClass()->getDeclaredMethod(
>>            JvNewStringUTF("hasNext"), NULL);
>>    printf("calling invoke, it'll dump core in natMethod.cc line 194\n");
>>    m->invoke(i, NULL);
>>    return 0;
>> }
>>
>> $gcc problem.cpp -lgcj
>> $./a.out
>> calling invoke
>> Aborted (core dumped)
>>
>> $gdb -core core a.out
>> (gdb) where
>> #0  0x00002adf9252bed5 in raise () from /lib/libc.so.6
>> #1  0x00002adf9252d3f3 in abort () from /lib/libc.so.6
>> #2  0x00002adf90bdeed8 in _Jv_Throw (value=0x2adf932cd370) at
>> ../../../src/libjava/exception.cc:128
>> #3  0x00002adf90bd2a2a in _Jv_catch_segv (_p=<value optimized out>) at
>> ../../../src/libjava/prims.cc:184
>> #4  <signal handler called>
>> #5  0x00002adf90c217d3 in java::lang::reflect::Method::invoke
>> (this=0x2adf932d1c80, obj=0x2adf93ba6e40,
>>    args=0x0) at ../../../src/libjava/java/lang/reflect/natMethod.cc:194
>> #6  0x0000000000400a5c in main ()
>>
>> The top of the stack is from the NULL-pointer catching signal handler
>> we believe, so #5 and #6 are the relevant ones.
>>
>> The point is that we believe that the scenario in the C++ code is
>> valid, both from Java and from C++, and we do not see the reasons for
>> the additional check that has been added to the invoke() method.
>
> Method.invoke() is required to check accessibility according to the
> Java Language Specification, so this check is required.

You refer to this I assume:
http://java.sun.com/docs/books/jls/third_edition/html/names.html#6.6

"A member (class, interface, field, or method) of a reference (class,
interface, or array) type or a constructor of a class type is
accessible only if the type is accessible and the member or
constructor is declared to permit access:

    * If the member or constructor is declared public, then access is
permitted. All members of interfaces are implicitly public.
    * Otherwise, if the member or constructor is declared protected,
then access is permitted only when one of the following is true:
                + Access to the member or constructor occurs from
within the package containing the class in which the protected member
or constructor is declared.
                + Access is correct as described in §6.6.2.
    * Otherwise, if the member or constructor is declared private,
then access is permitted if and only if it occurs within the body of
the top level class (§7.6) that encloses the declaration of the member
or constructor.
    * Otherwise, we say there is default access, which is permitted
only when the access occurs from within the package in which the type
is declared."

It seems that the type is accessible (Iterator) and the method is
declared public. Then it is accessible without further conditions.

The patch in invoke falls back to the scenario under the last bullet,
which is not correct I believe.

> The only thing libgcj is doing wrong is failing to check for null
> "caller" (which can't happen in Java code). It should probably do this
> check and throw an IllegalAccessException, or maybe just permit the
> access.

Yes, it should check for NULL and grant access, otherwise it would be
too restrictive.

> A workaround is to just add a Method.setAccessible() call to your code
> to bypass access checks.

Thanks, I'll remember that from my delegation in Java project and I'll
give it a try.

Erik
>
> Bryce

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Bryce McKinlay]

On Wed, Dec 23, 2009 at 3:58 PM, Erik J Groeneveld <erik@cq2.nl> wrote:

> You refer to this I assume:
> "A member (class, interface, field, or method) of a reference (class,
> interface, or array) type or a constructor of a class type is accessible
> only if the type is accessible and the member or constructor is declared to
> permit access:
>
> If the member or constructor is declared public, then access is permitted.
> All members of interfaces are implicitly public.
> Otherwise, if the member or constructor is declared protected, then access
> is permitted only when one of the following is true:
>
> Access to the member or constructor occurs from within the package
> containing the class in which the protected member or constructor is
> declared.
> Access is correct as described in §6.6.2.
>
> Otherwise, if the member or constructor is declared private, then access is
> permitted if and only if it occurs within the body of the top level
> class (§7.6) that encloses the declaration of the member or constructor.
> Otherwise, we say there is default access, which is permitted only when the
> access occurs from within the package in which the type is declared.
>
> "
> It seems that the type is accessible (Iterator) and the method is declared
> public. Then it is accessible without further conditions.
> The patch in invoke falls back to the scenario under the last bullet, which
> is not correct I believe.

In your example, you aren't testing the accessibility of Iterator, but
rather that of the class which is returned by ArrayList.iterator(),
which is indeed a package-private class from a different package!

Another fix for your code would be to change:

   java::lang::reflect::Method* m = i->getClass()->getDeclaredMethod(
           JvNewStringUTF("hasNext"), NULL);

to something like:

   java::lang::reflect::Method* m =
&(java::util::Iterator::class$)->getDeclaredMethod(
              JvNewStringUTF("hasNext"), NULL);

Bryce

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Erik Groeneveld]

On Wed, Dec 23, 2009 at 17:44, Bryce McKinlay <bmckinlay@gmail.com> wrote:
> On Wed, Dec 23, 2009 at 3:58 PM, Erik J Groeneveld <erik@cq2.nl> wrote:
[...]

> In your example, you aren't testing the accessibility of Iterator, but
> rather that of the class which is returned by ArrayList.iterator(),
> which is indeed a package-private class from a different package!

That is true. But the code that uses invoke is generic.  It calls a
given method on a given object:

java::lang::Object* callMethod(java::lang::Object* target,
java::lang::String* methodname)

It cannot do anything else than trying to find the method using the
java::lang::Object pointer that has been passed to it.  And it cannot
tell from this pointer that the actual reference the application is
using is of type Iterator (or any other interface for that matter).
This is something the compiler can work out, but not the runtime.
Indeed this works:

    java::util::ArrayList* l = new java::util::ArrayList();
    java::util::Iterator* i = l->iterator();
    i = l->iterator();
    i->hasNext();

But the fix below only works for the example I made up because a human
can tell the reference is of type Iterator:

> Another fix for your code would be to change:
>
>   java::lang::reflect::Method* m = i->getClass()->getDeclaredMethod(
>           JvNewStringUTF("hasNext"), NULL);
>
> to something like:
>
>   java::lang::reflect::Method* m =
> &(java::util::Iterator::class$)->getDeclaredMethod(
>              JvNewStringUTF("hasNext"), NULL);

So I wonder how the rules from 6.6 in the Java Language Specification
can be implemented properly when the type of the reference is not
known to invoke.  Because the relevant part of the spec speaks about
"members of a reference".

"A member (class, interface, field, or method) of a reference (class,
interface, or array) type or a constructor of a class type is
accessible only if the type is accessible and the member or
constructor is declared to permit access: ..."

but invoke simply does not know the reference, and cannot get it from
the Java call stack either (hence the core dump).  All it knows is the
actual object, and this object is of a type that is not accessible,
although it implements an interface that is accessible.

Is there a more gentle solution other than calling setAccessible() on
the method object? (which works)  Could it be done in GCJ?

Erik

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Andrew Haley]

On 12/23/2009 06:09 PM, Erik Groeneveld wrote:
> On Wed, Dec 23, 2009 at 17:44, Bryce McKinlay <bmckinlay@gmail.com> wrote:
>> On Wed, Dec 23, 2009 at 3:58 PM, Erik J Groeneveld <erik@cq2.nl> wrote:
> [...]
> 
>> In your example, you aren't testing the accessibility of Iterator, but
>> rather that of the class which is returned by ArrayList.iterator(),
>> which is indeed a package-private class from a different package!
> 
> That is true. But the code that uses invoke is generic.  It calls a
> given method on a given object:
> 
> java::lang::Object* callMethod(java::lang::Object* target,
> java::lang::String* methodname)
> 
> It cannot do anything else than trying to find the method using the
> java::lang::Object pointer that has been passed to it.  And it cannot
> tell from this pointer that the actual reference the application is
> using is of type Iterator (or any other interface for that matter).
> This is something the compiler can work out, but not the runtime.
> Indeed this works:
> 
>     java::util::ArrayList* l = new java::util::ArrayList();
>     java::util::Iterator* i = l->iterator();
>     i = l->iterator();
>     i->hasNext();

Right, because CNI is bypassing the controls.

> But the fix below only works for the example I made up because a human
> can tell the reference is of type Iterator:
> 
>> Another fix for your code would be to change:
>>
>>   java::lang::reflect::Method* m = i->getClass()->getDeclaredMethod(
>>           JvNewStringUTF("hasNext"), NULL);
>>
>> to something like:
>>
>>   java::lang::reflect::Method* m =
>> &(java::util::Iterator::class$)->getDeclaredMethod(
>>              JvNewStringUTF("hasNext"), NULL);
> 
> So I wonder how the rules from 6.6 in the Java Language Specification
> can be implemented properly when the type of the reference is not
> known to invoke.  Because the relevant part of the spec speaks about
> "members of a reference".
> 
> "A member (class, interface, field, or method) of a reference (class,
> interface, or array) type or a constructor of a class type is
> accessible only if the type is accessible and the member or
> constructor is declared to permit access: ..."
> 
> but invoke simply does not know the reference, and cannot get it from
> the Java call stack either (hence the core dump).  All it knows is the
> actual object, and this object is of a type that is not accessible,
> although it implements an interface that is accessible.
> 
> Is there a more gentle solution other than calling setAccessible() on
> the method object? (which works)  Could it be done in GCJ?

I can't see any good reason why there should be.  Unless the method
is marked accessible, java.lang.reflect.Method.invoke has to check the
class of the caller.  There should not be a segfault, true, but an
IllegalAccessException wouldn't be unreasonable.  That's what this
code does:

import java.lang.reflect.*;
import java.util.*;

public class Problem
{
  public static void main(String[] args)
    throws Throwable
  {
    ArrayList l = new ArrayList();
    Iterator i = l.iterator();
    Method m = (i.getClass().getDeclaredMethod
		("hasNext", null));
    m.invoke(i, null);
  }
}

Andrew.

Re: problem with class accessiblity check in invoke (natMethod.cc)

[Erik Groeneveld]

>>     java::util::ArrayList* l = new java::util::ArrayList();
>>     java::util::Iterator* i = l->iterator();
>>     i = l->iterator();
>>     i->hasNext();
>
> Right, because CNI is bypassing the controls.

Ok. CNI behaves just as both OpenJDK and GCJ do on corresponding Java code:

   ArrayList l = new ArrayList();
   Iterator i = l.iterator();
   i.hasNext();

And your example indeed raises IllegalAccessException in both OpenJDK and GCJ:

> import java.lang.reflect.*;
> import java.util.*;
>
> public class Problem
> {
>  public static void main(String[] args)
>    throws Throwable
>  {
>    ArrayList l = new ArrayList();
>    Iterator i = l.iterator();
>    Method m = (i.getClass().getDeclaredMethod
>                ("hasNext", null));
>    m.invoke(i, null);
>  }
> }

I am convinced but still unsure how to deal with it. I don't see a way
to translate the direct Java call into the corresponding reflection
part.  I think I settle for the setAccessible() option.

Best regards
Erik