Me trying to explain the problem behind #ReflectiveAccessToNonExportedTypes

[Remi Forax]

----- Mail original -----
> De: "David M. Lloyd" <david.lloyd at redhat.com>
> À: jpms-spec-experts at openjdk.java.net
> Envoyé: Jeudi 14 Juillet 2016 17:31:38
> Objet: Re: Me trying to explain the problem behind	#ReflectiveAccessToNonExportedTypes
> 
> On 07/14/2016 10:06 AM, Remi Forax wrote:
> > Let me try to explain again the problem and the possible solution.
> >
> > Let say i have two modules, module1 and module2.
> > In module1, I have two packages module1.exported and module1.nonexported,
> > so module1 has this module-info.java
> >
> >    module module1 {
> >      exports module1.exported;
> >    }
> >
> > in module1.nonexported, I have a class A that's implement an interface I
> > defined like this:
> >    package module1.nonexported;
> >
> >    import module1.exported.I;
> >
> >    public class A implements I {
> >      @Override
> >      public void foo() {
> >        System.out.println("foo");
> >      }
> >    }
> >
> > I is defined in module1.exported like this:
> >    package module1.exported;
> >
> >    import module1.nonexported.A;
> >
> >    public interface I {
> >      public void foo();
> >
> >      static I instance() {
> >        return new A();
> >      }
> >    }
> >
> > Now, in module2, i want to use I so module2 requires module1, like this:
> >    module module2 {
> >      requires module1;
> >    }
> >
> > and I have a main defined in the package modul2.main, like that:
> >    package module2.main;
> >
> > import java.lang.reflect.Method;
> >
> > import module1.exported.I;
> >
> > public class Main {
> >    public static void main(String[] args) throws
> >    ReflectiveOperationException {
> >      System.out.println(Class.forName("module1.nonexported.A"));
> >
> >      I i = I.instance();
> >      Method m = i.getClass().getMethod("foo");
> >      m.invoke(i);
> >    }
> > }
> >
> > If i run this code in with the two modules in the classpath,
> > everything work and the code prints
> >    class module1.nonexported.A
> >    foo
> >
> > Now, if if the two modules are in the modulepath,
> > Class.forName works but m.invoke() throws an IllegalAccessException because
> > A is in a non exported package
> >    java.lang.IllegalAccessException: class module2.main.Main (in module
> >    module2) cannot access class module1.nonexported.A (in module module1)
> >    because module module1 does not export module1.nonexported to module
> >    module2
> >
> > If i try with m.setAccessible(true), before m.invoke(), setAccessible
> > throws an exception InaccessibleObjectException
> >    java.lang.reflect.InaccessibleObjectException: Unable to make member of
> >    class module1.nonexported.A accessible:  module module1 does not export
> >    module1.nonexported to module module2
> >
> > A way to solve this issue, is to use the interface I to call foo instead of
> > using the class A,
> >    Method m = I.class.getMethod("foo");
> >    m.invoke(i);
> >
> > in that case, everything is Ok, because i don't try to use the method foo
> > of A anymore.
> >
> > What Mark as proposed is to introduce a new "export dynamic" semantics
> > which doesn't allow the package to be visible at compile time but to be
> > visible at runtime.
> > This solve the issue by supposing that everyone will write export dynamic
> > on every non exported package to be backward compatible with the Java 8
> > behavior.
> >
> > While i agree that "export dynamic" is a semantics that jigsaw should
> > provide, i disagree with the syntax "export dynamic" because for me, the a
> > non exported package should have this semantics in order to be backward
> > compatible with existing code.
> > So we should have 3 way to export or not a package,
> >    1. don't export at compile time, don't export at runtime
> >    2. don't export at compile time, export at runtime
> >    3. export at compile time, export at runtime.
> >
> > 2 should be the default, 1 should be use by the package of the JDK (or any
> > other libs) that want strong security, 3 is for the package that defines
> > API (that will be maintained forever).
> 
> This is a good summary and I think matches my understanding.  But the
> basic problem is that if everyone is doing this "export dynamic" then
> the safety proposed by restricting "public" is a false promise: we are
> providing the feature only to take it away again, resulting in the exact
> same situation we have today, but with no alternatives.  If we have to
> do this then I think the feature itself needs to be re-examined.
> 
> What I was suggesting was that we re-separate the notion of exports from
> accessibility altogether, so you again have three different situations
> rather than two:
> 
> A. Compile time
> B. Class link at run time
> C. Reflection at run time

B and C may be indistinguishable. For the OpenJdk + Hotspot, you can distinguish between access control done by the VM in C++ and access control done by the reflection API done at runtime. For OpenJDK + IBM J9, there is only one entry point for the the access checks. When specifying the JSR292 (java.lang.invoke + invokedynamic), we spend a lot of time to be sure that reflection behavior and bytecode behavior were aligned. I don't think that trying to separate B and C is possible that late in the game.

> 
> This changes the basic "export" back to mean "make available for
> linkage", with no bearing on accessibility.  Then you have (at minimum)
> the following ways to export (or not) a package:
> 
> 1. No export at compile time, no export but reflection-only at run time
> (but only for public types & members)
> 2. Yes export at compile time, yes export at run time (but only for
> public types & members)
> 3. Yes export at compile time and run time, including specific grants to
> access package-private members from targeted modules and/or packages
> 
> Note: We could add a 1½ which is an export at run time but not compile
> time if the EG agrees; I don't think that is particularly important
> though as build tools can accomplish this easily enough.  I don't want
> to derail the core point of the discussion on this topic, so I'm leaving
> it aside for now.

friend accesssibility doesn't solve the issue here, when you want 'strong encapsulation' i.e. no way to use reflection to access members of a not-exported class, you should not have to provide the list of all possible existing module. I think we want 'export private' (strong encapsulation) and 'export dynamic' to be available without having a friend list.
And again, changing the meaning of package private is a no go for me, by example for a package like java.lang.invoke, i want the package to be exported, i want strong encapsulation and i want some classes inside the package by example the class that implements the whole method handle hierarchy to share secret (using the package private access).

> 
> This way the Java 8 behavior is retained with no "export dynamic"
> anywhere in sight.  Rather you'd have something like "export private",
> which you'd only grant for those special cases where a package must be
> shared to a specific friend.  Things not intended to be shared can be
> safely migrated to the new friend package feature.

I agree that we should have a way to have Java 8 behavior without having to declare every packages with export dynamic,
see my discussion with mark about specifying a default export behavior.

> No new access limits
> are introduced - instead old ones are enhanced in what I think is a
> simple and intuitive way.  All the compatibility concerns go away, but
> we can still use the mechanism to seal up the problems in the JDK with
> shared code and things which should not be public.
> 
> We should retain setAccessible() for now for compatibility.  We could
> also introduce a "--safe-mode" flag which disables setAccessible()
> *across the board* (that is, not just for modular code) without
> controversy, because all those public members will remain accessible
> without "help", plus a way is provided to allow access to be volunteered
> from one package to another (along much the same philosophical lines as
> MethodHandles.Lookup IMO).

Deprecating setAccessible is another story for me.

> 
> The platform EG can then later decide if this safer mode is good for the
> platform, and if so, could move to making this the default and possibly
> adding an "--unsafe-mode" for a limited time.
> 
> --
> - DML
> 

regards,
Rémi