Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
The default implementation of ObjectInputStream::resolveClass and resolveProxyClass finds the user-defined class loader on the stack and assumes that only system classes are loaded by null loader. As JDK modules are deprivileged, classes on the stack defined by the platform class loader. These methods should be updated to prepare if any system class are defined by the platform class loader and its ancestors instead. As for the implementation, I fix JVM_LatestUserDefinedLoader to returns the first non-null class loader on the stack that is not platform class loader. This is so fragile and would be really nice if this could go away while the past work shows that it’s unlikely - Alan may say more on this. If this stays, I’d like this to be replaced with StackWalker API and remove such built-in logic in the VM in the future. Webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.00/ Mandy
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
For the record, I reviewed the jvm changes and they look good. thanks, Karen > On May 4, 2016, at 3:29 PM, Mandy Chung wrote: > > The default implementation of ObjectInputStream::resolveClass and > resolveProxyClass finds the user-defined class loader on the stack and > assumes that only system classes are loaded by null loader. As JDK modules > are deprivileged, classes on the stack defined by the platform class loader. > > These methods should be updated to prepare if any system class are defined by > the platform class loader and its ancestors instead. > > As for the implementation, I fix JVM_LatestUserDefinedLoader to returns the > first non-null class loader on the stack that is not platform class loader. > This is so fragile and would be really nice if this could go away while the > past work shows that it’s unlikely - Alan may say more on this. If this > stays, I’d like this to be replaced with StackWalker API and remove such > built-in logic in the VM in the future. > > Webrev: > http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.00/ > > Mandy
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
On 4 May 2016, at 20:29, Mandy Chung wrote:
> The default implementation of ObjectInputStream::resolveClass and
> resolveProxyClass finds the user-defined class loader on the stack and
> assumes that only system classes are loaded by null loader. As JDK modules
> are deprivileged, classes on the stack defined by the platform class loader.
>
> These methods should be updated to prepare if any system class are defined by
> the platform class loader and its ancestors instead.
>
> As for the implementation, I fix JVM_LatestUserDefinedLoader to returns the
> first non-null class loader on the stack that is not platform class loader.
> This is so fragile and would be really nice if this could go away while the
> past work shows that it’s unlikely - Alan may say more on this. If this
> stays, I’d like this to be replaced with StackWalker API and remove such
> built-in logic in the VM in the future.
That would be a good future enhancement.
> Webrev:
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.00/
The changes look good to me.
The wording for resolveProxyClass seems a little overly complicated,
and always has been. Would it be better to just take the same, more
direct, wording as resolveClass?
where loader is determined as follows: if there is a
method on the current thread's stack whose declaring class was
defined by a user-defined class loader (and was not a generated to
implement reflective invocations), then loader is class
loader corresponding to the closest such method to the currently
executing frame; otherwise, loader is the {@linkplain
ClassLoader#getPlatformClassLoader() platform class loader}.
-Chris.
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
On 04/05/2016 20:29, Mandy Chung wrote: The default implementation of ObjectInputStream::resolveClass and resolveProxyClass finds the user-defined class loader on the stack and assumes that only system classes are loaded by null loader. As JDK modules are deprivileged, classes on the stack defined by the platform class loader. These methods should be updated to prepare if any system class are defined by the platform class loader and its ancestors instead. As for the implementation, I fix JVM_LatestUserDefinedLoader to returns the first non-null class loader on the stack that is not platform class loader. This is so fragile and would be really nice if this could go away while the past work shows that it’s unlikely - Alan may say more on this. If this stays, I’d like this to be replaced with StackWalker API and remove such built-in logic in the VM in the future. Webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.00/ In resolveClass then "is class loader corresponding to the closest .." should probably be "is the class loader ...". You didn't introduce this of course, just noticed it while reading the complete paragraph. The rest looks okay to me, no objection to Chris's suggestion to re-word resolveProxyClass. -Alan
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
> On May 5, 2016, at 6:10 AM, Alan Bateman wrote:
>
> In resolveClass then "is class loader corresponding to the closest .." should
> probably be "is the class loader ...". You didn't introduce this of course,
> just noticed it while reading the complete paragraph.
>
> The rest looks okay to me, no objection to Chris's suggestion to re-word
> resolveProxyClass.
I was wondering why the resolveClass and resolveProxyClass methods are
specified differently w.r.t. class loader search. I made only localized change
as I didn’t have the history.
I’m happy to clean up the spec. I’d also fix the spec “user-defined class
loader” which isn’t correct as it also includes built-in app class loader.
* where loader is determined as follows: if there is a
* method on the current thread's stack whose declaring class is not a
*
* platform class (and was not a generated to implement
* reflective invocations), then loader is the class loader
* of such class; otherwise, loader is the {@linkplain
* ClassLoader#getPlatformClassLoader() platform class loader}.
Revised webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.01/
Mandy
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
On 05/05/2016 17:59, Mandy Chung wrote:
:
I was wondering why the resolveClass and resolveProxyClass methods are
specified differently w.r.t. class loader search. I made only localized change
as I didn’t have the history.
I’m happy to clean up the spec. I’d also fix the spec “user-defined class
loader” which isn’t correct as it also includes built-in app class loader.
* where loader is determined as follows: if there is a
* method on the current thread's stack whose declaring class is not a
*
* platform class (and was not a generated to implement
* reflective invocations), then loader is the class loader
* of such class; otherwise, loader is the {@linkplain
* ClassLoader#getPlatformClassLoader() platform class loader}.
Revised webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.01/
This looks okay to me. I noticed the existing spec also methods "class
loaders of generated reflection implementation classes" and not clear
that this should be in the normative text. Anyway, not your doing and
I'm suggesting we change it now.
-Alan.
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
On 5 May 2016, at 17:59, Mandy Chung wrote:
>
>> On May 5, 2016, at 6:10 AM, Alan Bateman wrote:
>>
>> In resolveClass then "is class loader corresponding to the closest .."
>> should probably be "is the class loader ...". You didn't introduce this of
>> course, just noticed it while reading the complete paragraph.
>>
>> The rest looks okay to me, no objection to Chris's suggestion to re-word
>> resolveProxyClass.
>
> I was wondering why the resolveClass and resolveProxyClass methods are
> specified differently w.r.t. class loader search. I made only localized
> change as I didn’t have the history.
>
> I’m happy to clean up the spec. I’d also fix the spec “user-defined class
> loader” which isn’t correct as it also includes built-in app class loader.
>
> * where loader is determined as follows: if there is a
> * method on the current thread's stack whose declaring class is not a
> *
> * platform class (and was not a generated to implement
> * reflective invocations), then loader is the class loader
> * of such class; otherwise, loader is the {@linkplain
> * ClassLoader#getPlatformClassLoader() platform class loader}.
>
> Revised webrev:
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.01/
Thanks Mandy, this version looks good.
-Chris.
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
On Thu, May 5, 2016 at 6:59 PM, Mandy Chung wrote:
> * where loader is determined as follows: if there is a
> * method on the current thread's stack whose declaring class is not a
> *
> * platform class (and was not a generated to implement
> * reflective invocations), then loader is the class loader
> * of such class; otherwise, loader is the {@linkplain
> * ClassLoader#getPlatformClassLoader() platform class loader}.
>
> Revised webrev:
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.01/
Hi,
"a generated to implement" should probably be fixed.
Regards
Florent
--
Florent Guillaume, Director of R&D, Nuxeo
Open Source, Java EE based, Enterprise Content Management (ECM)
http://www.nuxeo.com http://www.nuxeo.org +33 1 40 33 79 87
Re: Review Request JDK-8155977: Update ObjectInputStream::resolveClass and resolveProxyClass to work with platform class loader
> On May 5, 2016, at 3:50 PM, Florent Guillaume wrote:
>
> On Thu, May 5, 2016 at 6:59 PM, Mandy Chung wrote:
>> * where loader is determined as follows: if there is a
>> * method on the current thread's stack whose declaring class is not a
>> *
>> * platform class (and was not a generated to implement
>> * reflective invocations), then loader is the class loader
>> * of such class; otherwise, loader is the {@linkplain
>> * ClassLoader#getPlatformClassLoader() platform class loader}.
>>
>> Revised webrev:
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8155977/webrev.01/
>
> Hi,
>
> "a generated to implement" should probably be fixed.
In fact, I took out "(and was not a generated to implement reflective
invocations)” which is implementation details, followed up Alan’s comment.
webrev.01 is now updated in place (I should have done that earlier).
Mandy
