jdk.internal.reflect.ReflectionFactory and SecurityManager

[Claes Redestad]

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
>>