jdk.internal.reflect.ReflectionFactory and SecurityManager

[David Holmes]

Hi Claes,

On 27/12/2016 9:48 AM, Claes Redestad wrote:
> 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

At this stage I would categorise it as one possible way to work around 
the issue that JDK-8062389 et al. ran into.

The initialization sequence is fragile and is not a constant - different 
runtime options can affect the paths taken. So testing anything that 
introduces a change to initialization order is tricky, as there is no 
obvious set of tests that provide full coverage.

Cheers,
David

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