On 2016-12-26 21:39, Peter Levart wrote:
Hi Claes,
On 12/26/2016 09:11 PM, Claes Redestad wrote:
Hi,
with strong encapsulation in place this seems perfectly fine to me.
The only downside is that we will lose the extra reminder that the
ReflectionFactory must not escape to untrusted code, but I think we can
all help ensure that doesn't happen, right? :-)
With strong encapsulation in place, if the ReflectionFactory escaped, it
could only be used by the following modules:
exports jdk.internal.reflect to
java.corba,
java.logging,
java.sql,
java.sql.rowset,
jdk.dynalink,
jdk.scripting.nashorn,
jdk.unsupported;
If SecurityManager check was removed, those modules could then also
obtain ReflectionFactory (they currently don't use it except
jdk.unsupported). Does this present any new danger?
Right, it shouldn't be possible to do anything with a reference to an
encapsulated object, but let's not challenge this assumption here and
now.
Changes needed to support removal of this SecurityManager check are as
follows:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ReflectionFactory.withoutSM/webrev.01/
Changes mostly remove the need to store ReflectionFactory into a
variable, therefore they encourage style where ReflectionFactory is less
likely to escape.
That's a good point, and changes looks good to me. I think this could go
as both a standalone cleanup and rolled into a redo of 8062389 etc.
Thanks!
/Claes
Regards, Peter
Thanks!
/Claes
On 2016-12-26 20:29, 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
