RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields

Sean Mullan
Please review this change to remove the pre-JDK 1.2 SecurityManager
methods that have been deprecated since JDK 1.2 and marked for removal
in JDK 9. These methods are fragile, error-prone and have been obsolete
since the SecurityManager was revamped in JDK 1.2. The methods to be
removed are: getInCheck, classDepth, classLoaderDepth,
currentClassLoader, currentLoadedClass, inClass, and inClassLoader.

In addition, the deprecated and error-prone checkMemberAccess method
(which was deprecated in JDK 8 and marked for removal in JDK 9) has been
changed to throw SecurityException if the caller has not been granted
AllPermission. This makes the method less likely it will be used
incorrectly while still allowing some more time before it is removed.

http://cr.openjdk.java.net/~mullan/webrevs/8186535/webrev.00/

Thanks,
Sean
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields

Alan Bateman
On 22/11/2017 14:37, Sean Mullan wrote:

> Please review this change to remove the pre-JDK 1.2 SecurityManager
> methods that have been deprecated since JDK 1.2 and marked for removal
> in JDK 9. These methods are fragile, error-prone and have been
> obsolete since the SecurityManager was revamped in JDK 1.2. The
> methods to be removed are: getInCheck, classDepth, classLoaderDepth,
> currentClassLoader, currentLoadedClass, inClass, and inClassLoader.
>
> In addition, the deprecated and error-prone checkMemberAccess method
> (which was deprecated in JDK 8 and marked for removal in JDK 9) has
> been changed to throw SecurityException if the caller has not been
> granted AllPermission. This makes the method less likely it will be
> used incorrectly while still allowing some more time before it is
> removed.
>
> http://cr.openjdk.java.net/~mullan/webrevs/8186535/webrev.00/
This mostly looks good.

Does the stack walker created in AppletSecurity need to be done in a
privileged block? If this is just the mouldy appletviewer tool then
ignore my comment.

A minor comment is that the <code> is legacy and we've been using {@code
...} for recent changes. We changed some of these methods to use {@code
...} when we degraded them in JDK 9. Also NoAWT probably isn't the right
place to test checkMemberAccess. If the test is renamed and the
description changed then it would be okay.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields

Sean Mullan
On 11/22/17 9:59 AM, Alan Bateman wrote:

>> http://cr.openjdk.java.net/~mullan/webrevs/8186535/webrev.00/
> This mostly looks good.
>
> Does the stack walker created in AppletSecurity need to be done in a
> privileged block? If this is just the mouldy appletviewer tool then
> ignore my comment.

Hmm. Where do you see it being called inside doPrivileged?

> A minor comment is that the <code> is legacy and we've been using {@code
> ...} for recent changes. We changed some of these methods to use {@code
> ...} when we degraded them in JDK 9.

I guess you are referring to the checkMemberAccess method. Sure I can
change it to use {@code}.

> Also NoAWT probably isn't the right
> place to test checkMemberAccess. If the test is renamed and the
> description changed then it would be okay.

Sure, the test already had everything I need so it was easier to
leverage it rather than just duplicating. I had changed the description
already. I will just rename it to something more generic, like
DepMethodsRequireAllPerm.

--Sean
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields

Alan Bateman


On 22/11/2017 15:49, Sean Mullan wrote:
>
> Hmm. Where do you see it being called inside doPrivileged?
StackWalker.getInstance does a permission check when you want class refs.


> :
>
> Sure, the test already had everything I need so it was easier to
> leverage it rather than just duplicating. I had changed the
> description already. I will just rename it to something more generic,
> like DepMethodsRequireAllPerm.
>
I think I added the NoAWT test for the checkXXX methods that used to
check AWTPermission, hence the name. Renaming it would be good.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields

Mandy Chung
In reply to this post by Sean Mullan


On 11/22/17 6:37 AM, Sean Mullan wrote:
Please review this change to remove the pre-JDK 1.2 SecurityManager methods that have been deprecated since JDK 1.2 and marked for removal in JDK 9. These methods are fragile, error-prone and have been obsolete since the SecurityManager was revamped in JDK 1.2. The methods to be removed are: getInCheck, classDepth, classLoaderDepth, currentClassLoader, currentLoadedClass, inClass, and inClassLoader.

In addition, the deprecated and error-prone checkMemberAccess method (which was deprecated in JDK 8 and marked for removal in JDK 9) has been changed to throw SecurityException if the caller has not been granted AllPermission. This makes the method less likely it will be used incorrectly while still allowing some more time before it is removed.

http://cr.openjdk.java.net/~mullan/webrevs/8186535/webrev.00/


src/java.desktop/share/classes/sun/applet/AppletSecurity.java
 111     private static final StackWalker walker =
 112         StackWalker.getInstance(RETAIN_CLASS_REFERENCE);

This call will do a stack-based permission check.  So it needs to be wrapped with doPrivileged.

Otherwise, looks fine.

Just to mention this:  AppletSecurity does not really need the currentClassLoader method.   AppletSecurity::currentAppletClassLoader could be reimplemented to use StackWalker to walk the stack once (replacing the call to currentClassLoader and getClassContext) to find AppletClassLoader.   OTOH it does not worth making more change since applets are going away.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields

Sean Mullan
On 11/28/17 2:41 PM, mandy chung wrote:

>
>
> On 11/22/17 6:37 AM, Sean Mullan wrote:
>> Please review this change to remove the pre-JDK 1.2 SecurityManager
>> methods that have been deprecated since JDK 1.2 and marked for removal
>> in JDK 9. These methods are fragile, error-prone and have been
>> obsolete since the SecurityManager was revamped in JDK 1.2. The
>> methods to be removed are: getInCheck, classDepth, classLoaderDepth,
>> currentClassLoader, currentLoadedClass, inClass, and inClassLoader.
>>
>> In addition, the deprecated and error-prone checkMemberAccess method
>> (which was deprecated in JDK 8 and marked for removal in JDK 9) has
>> been changed to throw SecurityException if the caller has not been
>> granted AllPermission. This makes the method less likely it will be
>> used incorrectly while still allowing some more time before it is
>> removed.
>>
>> http://cr.openjdk.java.net/~mullan/webrevs/8186535/webrev.00/
>>
>
> src/java.desktop/share/classes/sun/applet/AppletSecurity.java
>   111     private static final StackWalker walker =
>   112         StackWalker.getInstance(RETAIN_CLASS_REFERENCE);
>
> This call will do a stack-based permission check.  So it needs to be
> wrapped with doPrivileged.

Yes, Alan had the same comment. I have wrapped it in doPrivileged.

> Otherwise, looks fine.
>
> Just to mention this:  AppletSecurity does not really need the
> currentClassLoader method. AppletSecurity::currentAppletClassLoader
> could be reimplemented to use StackWalker to walk the stack once
> (replacing the call to currentClassLoader and getClassContext) to find
> AppletClassLoader. OTOH it does not worth making more change since
> applets are going away.

Ok. Good point though.

--Sean