Skip to content

Make default constructors accessible from witin IntrinsicObjectModel.constructWithin#4

Open
simonis wants to merge 2 commits intoObjectLayout:masterfrom
simonis:fix_default_constructor_visibility
Open

Make default constructors accessible from witin IntrinsicObjectModel.constructWithin#4
simonis wants to merge 2 commits intoObjectLayout:masterfrom
simonis:fix_default_constructor_visibility

Conversation

@simonis
Copy link

@simonis simonis commented Nov 11, 2014

If constructing intrinisc fields with "IntrinsicMembers.constructWithin(<field_name>, this)" we have to make the corresponding default constructor of the field's class accessible in IntrinsicObjectModel.constructWithin() otherwise the construction may fail.

@giltene
Copy link
Member

giltene commented Dec 11, 2014

I am trying to avoid making constructors accessible this way (and will likely take away similar behavior in StructuredArray). See more discussion in #6 for details.

…ObjectModel.constructWithin() we have to ensure that they are also accessible at the call site of IntrinsicObjectModel.constructWithin(). In other words, IntrinsicObjectModel.constructWithin() should only be able to call a constructor, if this constructor was also callable at the point where we actually initialize an @Intrinisc field.
@simonis
Copy link
Author

simonis commented Dec 11, 2014

Hi Gil,
so if I understand you right, your main concerns are security, because setAccessible() removes all the access restrictions from a constructor.
I had the same concenrns and I solved them in another change (see above). Sorry for pushing it that late . should have done that together with the initial pull request.
So the second change not only does additionl checking on the "this" argument of constructWithin (otherwise we could pass and inanother object and change it under the hood), but it also validates that the constructor (not only the default constructor) is really accessible at the call site of constructWithin().
It does this by using Reflection.ensureMemberAccess(). In order to use this method, the ObjectLayout library has to be in the boot class path, but I think that's no problem as we want to bring it into the standard library anyway. For now to work I've added the corresponding -Xbootclasspath option to the test exectuon and as far as I can see, everything still works.

By the way, we should add some tests (positive and negative) which ensure that we only the accessible constructors can be called and vice versa and that the this parameter of constructWithin is the right object. I may do that with a follow-up change soon.

@giltene
Copy link
Member

giltene commented Dec 11, 2014

Hi Volker,

See followup comment in the issue (#6). I've captured it there so that we'll have it as part of the overall problem we are trying to solve.

The two problems I see with the solution captured here this far is:

  1. bootclasspath requirement (explained in the issue comment)
  2. use of sun.reflect.Reflection.getCallerClass() [not detailed in the issue comment].

There are two potential problems I see with using sun.reflect.Reflection.getCallerClass():

a. Oracle is planning to deprecate it. (e.g. discussions in http://www.infoq.com/news/2013/07/Oracle-Removes-getCallerClass and http://bugs.java.com/view_bug.do?bug_id=8014925).

b. It is potentially unreliable in the presence of instrumentation (may need to walk the stack beyond immediate caller, etc.).

@simonis
Copy link
Author

simonis commented Dec 12, 2014

Hi Gil,

I don't think your point 2 is a real problem. What you are referring to is "getCallerClass(int realFramesToSkip)". It was indeed removed in jdk8 and one of the 7u updates. But it was replaced by "getCallerClass()" and the "CallerSensitive" annotation which are now available in Java 7 to 9 (see change 8010117: Annotate jdk caller sensitive methods with @sun.reflect.CallerSensitive). It is the new (and safe) way of getting the caller class as proposed by JEP 176.

The problem with the new version is that only system classes can use it (and the corresponding annotation). That's the reason why the ObjectLayout classes have to be placed in the boot class path now.

Regarding your point 1 I agree that it is unfortunate, but honestly speaking I think that ObjectLayout only makes real sense if we get it intrinsified. So if we have to change the VM anyway, it should also be no problem to place the ObjectLayout classes into the boot class path.

@giltene
Copy link
Member

giltene commented Dec 12, 2014

Re: point 1 first:

A "prime directive" of org.ObjectLayout is that the only difference observable between an intrinsified implementation and one that is "vanilla" is speed (and potentially space). Acceptable exceptions to this are the observations of internal implementation detail by unsafe probing things, like Unsafe, and like reflection-drilling into private or otherwise not-supposed-to-be-visible details of the implementation.

While it is arguable that once org.ObjectLayout crosses into the JDK, this prime directive will be met, I believe that the needs to meet it exists in the vanilla form outside of the JDK (the org.ObjectLayout namespace), and that it must be met without requiring any JVM flags (including bootclasspath). The strong reason for this is that our goal is to mature org.ObjectLayout's vanilla implementation to the point where it can be put into the JDK spec (in vanilla, no intrinsified form) without risk. For that to happen, we need it to be completely useable on existing JDKs. And usable means more than "demonstrable". It needs to actually work in real and deployable code.

Re: point 2. The very fact that getCallerClass(int) was replaced by getCallerClass() within the same major Java SE implementation of a specific JDK demonstrates why it cannot be safely used in this context. The current JEP 176 is OpenJDK-specific, and does not provide any sort of spec reliance.

A simple way to look at it is this: We have not yet shown a way to reliably make the logic of the current API use inaccessible constructors on Java SE compliant JDKs without breaking encapsulation in an unacceptable way. While a OpenJDK-specific (and OpenJDK-version-specific) way exists, it may not work on some JDKs, and may not work with some frameworks (e.g. ones that may insert interposing methods into the call chain).

I believe that an API change can resolve this issue in a way that would reliably work on any Java SE 7 (or later) JDK without requiring any JVM flags. My intent is to try to make that happen using by using a MethodHandle as an optional way to specify a constructor when constructing a CtorAndArgs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants