You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jdo-dev@db.apache.org by Martin Zaun <Ma...@Sun.COM> on 2005/10/12 08:07:39 UTC
class loader issue in java model - ri11 security manager tests failing
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());
>>
>>> }
Re: class loader issue in java model - ri11 security manager tests
failing
Posted by Michael Bouschen <mb...@spree.de>.
Hi Martin,
thanks for testing! All your remarks make sense. I changed the code
accordingly and checked it in.
Regards Michael
>
> Michael,
> I tested your JDO1 patch with JDK 1.4 in a clean workspace, and
> all security manager tests run fine.
>
> I mostly looked at the code additions to RuntimeJavaModel.java,
> which are well documented (no further questions on the code).
> Also, the getJavaModel() and getDeclaringJavaModelFactory()
> changes look good. I was surprised about the amount of changes,
> but to me the classloading code has gained clarity.
>
> Only true cosmetics:
> - 'instacne' misspelled twice in RuntimeJavaModel
> - "class instance" might be ambiguous, consider "Class instance"
> since the comments refer to an instance of java.lang.Class
> - The 'true' value passed to the two invocations of forNamePrivileged()
> leaves one guessing, though the code comments explain the intent.
> Consider:
> final boolean initialize = true;
> RuntimeJavaModelFactory.forNamePrivileged(..., initialize,...)
> - In getJavaType(Class), somewhere after
> if (javaType == null) { ...
> I'd expected a comment (or assertion) about the special handling
> of pre-defined types, i.e., that 'javaType' cannot be a PreDefined
> type. Correct? The javadoc on that class already has it, so it's
> not a real lack of information there, just redundancy.
>
> Martin
>
> Michael Bouschen wrote:
>
>>
>> I have implemented the JavaModel changes as we discussed. Now the
>> Class.forName is called in a doPrivileged block to avoid the
>> SecurityException.
>> The other change is that I moved all the code that checks for the
>> right class loader to the RuntimeJavaModel. A JavaType lookup by name
>> first gets the class instance using the class loader bound to the
>> model instance. Then it gets the class loader from the class instance
>> and delegates to JavaModel bound to this class loader. This makes
>> sure that a JavaModel handles only classes that are loaded by th
>> class loader bound to the JavaModel. There is one exception: all
>> JavaModel instances know about all the PredefinedType instances
>> (JavaType instances for primitive types, Java wrapper class,
>> java.util classes etc.).
>>
>> Attached find two patches created from the trunk(!) directory:
>> - JDO1-JavaModel.patch: ri11 changes. I tested this running the ri11
>> tests and tck11 in a jdk1.5 environment (I'm using the enhancer
>> changes you send over).
>> - JDO2-JavaModel.patch: the corresponding changes for JDO2. This
>> changes classes in core20, enhancer20, runtime20 and fostore20. I run
>> the fostore20 tests with jdk 1.4.
>>
>> Please let me know what you think.
>
--
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
Re: class loader issue in java model - ri11 security manager tests
failing
Posted by Martin Zaun <Ma...@Sun.COM>.
Michael,
I tested your JDO1 patch with JDK 1.4 in a clean workspace, and
all security manager tests run fine.
I mostly looked at the code additions to RuntimeJavaModel.java,
which are well documented (no further questions on the code).
Also, the getJavaModel() and getDeclaringJavaModelFactory()
changes look good. I was surprised about the amount of changes,
but to me the classloading code has gained clarity.
Only true cosmetics:
- 'instacne' misspelled twice in RuntimeJavaModel
- "class instance" might be ambiguous, consider "Class instance"
since the comments refer to an instance of java.lang.Class
- The 'true' value passed to the two invocations of forNamePrivileged()
leaves one guessing, though the code comments explain the intent.
Consider:
final boolean initialize = true;
RuntimeJavaModelFactory.forNamePrivileged(..., initialize,...)
- In getJavaType(Class), somewhere after
if (javaType == null) { ...
I'd expected a comment (or assertion) about the special handling
of pre-defined types, i.e., that 'javaType' cannot be a PreDefined
type. Correct? The javadoc on that class already has it, so it's
not a real lack of information there, just redundancy.
Martin
Michael Bouschen wrote:
>
> I have implemented the JavaModel changes as we discussed. Now the
> Class.forName is called in a doPrivileged block to avoid the
> SecurityException.
> The other change is that I moved all the code that checks for the right
> class loader to the RuntimeJavaModel. A JavaType lookup by name first
> gets the class instance using the class loader bound to the model
> instance. Then it gets the class loader from the class instance and
> delegates to JavaModel bound to this class loader. This makes sure that
> a JavaModel handles only classes that are loaded by th class loader
> bound to the JavaModel. There is one exception: all JavaModel instances
> know about all the PredefinedType instances (JavaType instances for
> primitive types, Java wrapper class, java.util classes etc.).
>
> Attached find two patches created from the trunk(!) directory:
> - JDO1-JavaModel.patch: ri11 changes. I tested this running the ri11
> tests and tck11 in a jdk1.5 environment (I'm using the enhancer changes
> you send over).
> - JDO2-JavaModel.patch: the corresponding changes for JDO2. This changes
> classes in core20, enhancer20, runtime20 and fostore20. I run the
> fostore20 tests with jdk 1.4.
>
> Please let me know what you think.
Re: class loader issue in java model - ri11 security manager tests
failing
Posted by Michael Bouschen <mb...@spree.de>.
Hi Martin,
I have implemented the JavaModel changes as we discussed. Now the
Class.forName is called in a doPrivileged block to avoid the
SecurityException.
The other change is that I moved all the code that checks for the right
class loader to the RuntimeJavaModel. A JavaType lookup by name first
gets the class instance using the class loader bound to the model
instance. Then it gets the class loader from the class instance and
delegates to JavaModel bound to this class loader. This makes sure that
a JavaModel handles only classes that are loaded by th class loader
bound to the JavaModel. There is one exception: all JavaModel instances
know about all the PredefinedType instances (JavaType instances for
primitive types, Java wrapper class, java.util classes etc.).
Attached find two patches created from the trunk(!) directory:
- JDO1-JavaModel.patch: ri11 changes. I tested this running the ri11
tests and tck11 in a jdk1.5 environment (I'm using the enhancer changes
you send over).
- JDO2-JavaModel.patch: the corresponding changes for JDO2. This changes
classes in core20, enhancer20, runtime20 and fostore20. I run the
fostore20 tests with jdk 1.4.
Please let me know what you think.
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 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
Re: class loader issue in java model - ri11 security manager tests
failing
Posted by Martin Zaun <Ma...@Sun.COM>.
Michael,
Michael Bouschen wrote On 10/12/05 12:18,:
> 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.
Thursday 7:30-9 am would be OK for me. Please, call me at home.
Thx,
Martin
Re: class loader issue in java model - ri11 security manager tests
failing
Posted by Michael Bouschen <mb...@spree.de>.
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
Re: class loader issue in java model - ri11 security manager tests
failing
Posted by Martin Zaun <Ma...@Sun.COM>.
Martin Zaun wrote:
>
> Hi Michael, ...
Sorry, I meant Michael B. :)
Martin