Hi,
while perhaps an enhancement in isolation, I'll argue this to be a
blocker of or a sub-task of the redo of JDK-8062389 - a P2 bug that
has been long in the making - thus I don't agree that the time for this
has passed, and neither do I think this needs a critical approval
request if it's pushed to enable a redo of JDK-8062389 et al.
Thanks!
/Claes
On 2016-12-26 22:16, David Holmes wrote:
Hi Peter,
I know this is response to the problems with your other recent change,
but this is an enhancement in my opinion, not a bug fix, and the time
for enhancements is passed - though there is still a process to request
them. I do not like to see last minutes changes like this where not
enough due diligence may be done to identify all the ramifications.
The Class.getMethod() fixes seem to have overstepped the line in that
regard as well, in my opinion. Anything that potentially changes
initialization order is fragile and risky and requires additional testing.
That said, I am an admirer of your work on OpenJDK! :)
Cheers,
David
On 27/12/2016 5:29 AM, Peter Levart wrote:
Hi,
There are 2 ReflectionFactory classes in JDK 9. The old one is
sun.reflect.ReflectionFactory which ended in jdk.unsupported module and
to which access is restricted with SecurityManager. There is also new
jdk.internal.reflect.ReflectionFactory which is used internally by JDK,
is exported to internal modules only but still uses SecurityManager to
restrict access to itself. I checked all usages and they all use
AccessControler.doPrivileged() for obtaining the instance of
jdk.internal.reflect.ReflectionFactory, which somehow defeats the
purpose of SecurityManager access checks in this API.
I think this could be simplified by removing the SecurityManager check
from jdk.internal.reflect.ReflectionFactory#getReflectionFactory static
method and change all usages to invoke this method directly without
doPrivileged(). There are already two sensitive internal APIs exposed
without SecurityManager checks: jdk.internal.misc.Unsafe#getUnsafe and
various jdk.internal.misc.SharedSecrets#getXxxAccess methods. So why
wouldn't internal ReflectionFactory be exposed the same way?
This would make obtaining the ReflectionFactory more robust and not
sensitive to bootstrap issues that surfaced recently after my push of a
fix for issues 8062389, 8029459, 8061950.
So, what do you think? Is this a worthwhile cleanup and simplification?
Regards, Peter
