Mailing-List: contact jdo-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: jdo-dev@db.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Message-ID: <434D616D.1060304@spree.de>
Date: Wed, 12 Oct 2005 21:18:05 +0200
From: Michael Bouschen <mbo.tech@spree.de>
Organization: Tech@Spree Engineering
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
 rv:1.7.3) Gecko/20040910
MIME-Version: 1.0
To: jdo-dev@db.apache.org
Subject: Re: class loader issue in java model - ri11 security manager tests
 failing
References: <432AD702.8010000@sun.com> <432ADABE.7090608@spree.de>
 <4333252B.5060507@sun.com> <4333741D.2040203@sun.com>
 <4334545F.1040503@sun.com> <433BB747.3070707@spree.de>
 <433BDF89.1090301@sun.com> <433BF9EB.8090506@spree.de>
 <434CA82B.4060405@sun.com>
In-Reply-To: <434CA82B.4060405@sun.com>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Hi Martin,

thanks for the info! Maybe it makes sense to discuss this over the 
phone. I have a meeting from 9-10am PDT tomorrow, so we could talk 
before 9am (which is a little early for you) or on Friday.

Regards Michael

> 
> 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());
>>>
>>>>              }


-- 
Michael Bouschen		Tech@Spree Engineering GmbH
mailto:mbo.tech@spree.de	http://www.tech.spree.de/
Tel.:++49/30/235 520-33		Buelowstr. 66			
Fax.:++49/30/2175 2012		D-10783 Berlin