db-jdo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Zaun <Martin.Z...@Sun.COM>
Subject class loader issue in java model - ri11 security manager tests failing
Date Wed, 12 Oct 2005 06:07:39 GMT

Hi Michael,

unfortunately, the fix for the class registration problem, as we
discussed it and as it was checked in, causes most of the ri11
security manager junit tests to fail (138 out of 149 tests).

The symptom is an AccessControlException ("getClassLoader") within
method Class.forName(), called from ReflectionJavaModel.java:130.

The issue is exposed by an almost trivial change that we discussed
(see below): When calling Class.forName() to initialize a class, we
decided to use the class' classloader instead of the one stored in
the JavaModel instance.  As you pointed out, we then must fetch the
classloader in a doPrivileged block.  (This block succeeds, the
exception is really raised within Class.forName()).

Though the code assumes that the stored and the fetched classloader
are always the same here, we discussed that if they're not, this
would result in an internal error that is very hard to track down,
since the consequences might show up later and elsewhere depending
on the order in which classes are registered.  We considered to
guard against this case with an assertion or an explicit check.

It turns out the stored and the fetched classloader may differ for
two reasons, I think.

First, clazz.getClassLoader() returns null for a number of classes
like Date and ArrayList.  Craig has pointed out that this is the
specified behaviour for classes loaded by the bootstrap classloader.

The javadoc on Class.forName() says that if the loader argument is
null (and other conditions apply), the security manager is checked
for permission ("getClassLoader") to access the bootstrap class
loader.  This check fails.

When using the stored classloader, which is always non-null, instead
of the fetched one as argument to clazz.forName(), this method does
not issue a "getClassLoader" request, and all tests security manager
junit tests pass then.

But I'm not sure this is the right solution either.  Essentially,
we'd represent Date, ArrayList etc. a multiple times in the Java
model - in each model instance per application classloader - while
they're only represented once in the JVM, by the bootstrap loader.

There might be another case to consider why the stored and the
fetched classloader may differ:  An application that uses multiple
classloaders forming a parent-child hierarchy, it may happen that
a PC class is loaded by a child classloader while it's superclasses
or the persistent field types are loaded by a parent classloader.

In this case, we probably do not want to have the type universe be
duplicated in each java model instance (i.e. per classloader), but
rather represent a class in the java model only as many times as
it's loaded in the VM, that is, per "owning" classloader.

Now, I do not know if we're already doing this (class represented in
java model only for "owning" classloader), but it seems to me that
the implicit assumption in ReflectionJavaModel.getJavaType() that the
stored and the fetched classloader are always the same does not hold.

That's why I'm not sure that always using the stored classloader
(instead of the fetched one) would be a general solution either.

Any thoughts?

Sorry for the long email.  If you'd rather like to discuss details
over phone, I'd have some time on Thursday morning (until 9:30am) or
on Friday during or after our JDO phone con.  Craig and I discussed
this issue briefly today.

Thx,
Martin

Michael Bouschen wrote:
> 
> About the issue of a class argument of a wrong classloader passed to the 
> JavaModel method getJavaType: I agree the code should use the class 
> loader from the class object. The only issue is that the call 
> clazz.getClassLoader() might result in a SecurityException, so I need to 
> put this call into a doPrivileged block. I already have a convenience 
> method in RuntimeJavaModelFactory, I just need to move this method to a 
> class that is accessible in both places.
>>
>> Michael Bouschen wrote:
>>
>>> Hi Martin,
>>>
>>> attached you find a patch for the JDOModel implementation. It 
>>> initializes a class instance if the model instance runs with the 
>>> initialize=true flag. It also fixes the MissingResourceException. I 
>>> could successfully run the ri tests in a workspace including your 
>>> enhancer plus my JDOModel changes.
>>> I would port the JDOModel changes to core20 and check them in, if you 
>>> agree with the changes.
>>>
>>> trunk/ri11/src/java/org/apache/jdo/impl/model/java/reflection/ReflectionJavaModel.java

>>>      public JavaType getJavaType(Class clazz)
>>>      {
>>> -        String name = clazz.getName();
>>> -        synchronized (types) {
>>> -            JavaType javaType = (JavaType)types.get(name);
>>> -            if (javaType == null) {
>>> -                javaType = createJavaType(clazz);
>>> -                types.put(name, javaType);
>>> +        if (clazz == null)
>>> +            return null;
>>> +        +        if (initialize) {
>>> +            try {
>>> +                // make sure the class is initialized
>>> +                Class.forName(clazz.getName(), initialize, 
>>> classLoader);
>>
>>
>> Since this is a public method, there's a chance for a bug that we're
>> called here with a class argument of a wrong classloader, I think.
>> If that happened, we'd load and initialize the class in the wrong
>> place.  So, I'd change this line, making an assumption explicit, to:
>>                 Class.forName(clazz.getName(), initialize, 
>> clazz.getClassLoader());
>> Or, if we allowed for assertions, we could keep the line but just add:
>>                 assert (classLoader == clazz.getClassLoader());
>>
>>>              }

Mime
View raw message