On 09/02/2013 06:45 PM, Nick Williams wrote:
On Sep 2, 2013, at 9:59 AM, Alan Bateman wrote:
It was a non goal of JEP 176 to provide @CallerSensitive as a public API. So
the suggestion to start out small was to leave that out and focus on some of
the use-cases initially. I don't think this suggestion is unreasonable as it
allows the less controversial part to move forward.
To be blunt, this goal is incompatible with the need to make getCallerClass
public. Either we need to remove CallerSensitive entirely (which I have nothing
against), or we need to make it public. But we can't keep CallerSensitive
private and make getCallerClass public. Now if you just have a general
objection to making getCallerClass public, then please, state it.
Sorry, I don't understand why this is incompatible. Other public
platform classes are annotated with @CS without it being made public.
Why the difference here?
I quickly skimmed the patch to see if could get the answer to my above
question. I could be wrong, but it appears that you expose the native
methods that wrap the calls into the VM directly, and expect users of
these methods to be annotated with @CS. I know it may not be ideal, but
could similar functionality be achieved by not exposing these methods
directly? Say having a non-native public method call a private
implementation method that calls into the VM. There may be some knock on
changes in the VM to support this, but it could be an option?
-Chris.
In fact, if anything, I'm actually in favor of removing CallerSensitive
entirely. It prevents code compiled on Java 6 from using those two methods,
since they can't be annotated with @CallerSensitive.
On Sep 2, 2013, at 3:33 AM, Alan Bateman wrote:
From a quick scan then it appears that you've decided to ignore the security
concerns so I don't think anyone can (or should) sponsor this patch until there
is further discussion on the API and the security implications.
To help things then one suggestion is start small and just focus on the no-arg
getCallerClass and the method to return the array of stack traces (be it an
extended StackTraceElement or the new StackTraceFrame that you are proposing).
The getCallerClass will need a permission check, as will the method to get the
extended stack trace. I think it would also be useful to lay out the points on
extending StackTraceEment vs. introducing a few StackTraceFrame type.
I didn't decide to ignore the security concerns, I just don't see any security
concerns. As has been /clearly/ established in the last three months,
frameworks have been using getCallerClass for years, if not a decade. It has
never caused a security problem. Ever. If I'm wrong, please point out to me a
specific vulnerability that this has led to.
I have worked on this API for a MONTH. We discussed it for TWO months before
that. I am a month behind on the deadline for my Java 8 / Java EE 7 book
because I essentially dropped it to work on this. I don't have the ability to
go back and start from the beginning on something that I had the understanding
was generally agreed upon before I started. It's simply not an option. We
/clearly/ outlined these use cases in multiple emails over the last three
months:
1. The need to get the immediate caller class.
2. The need to get the caller class N levels back.
3. The need to get the entire current stack trace, except with Classes instead
of class names.
4. The need to get the stack trace for a Throwable, except with Classes instead
of class names.
This API meets all of these needs. The one you suggest us starting over on does
not. It also adds the ability to get single frames from the stack trace,
because that's a natural extension of the above four use cases, and it was
trivial to include. There is /no/ need to lay out the points of extending
StackTraceElement versus adding StackTraceFrame. It was made /very/ clear to me
by several people in June that changing/extending StackTraceElement was /not/
an option due to serialization issues. The very clear intent was for this
change to have minimal/no impact on the existing APIs, and that's what I did.
The exact interface I used for StackTraceFrame was present in many previous
emails (and evolved over about a month of communications), and when I said that
I was starting on a patch there were no specific objections to the API that I
ever saw. Jörn's email links to several of these discussions.
I'm not voicing any objection to any kind of security/permissions checks in
these methods. Before I can pass judgement one way or another, I'd want to know
1) specifically what type of permissions check you are looking for, and 2) what
you're looking to achieve with said permissions check.
In general, doing things like instantiating a class or calling methods on a
class using the Class involves security checks already. I don't see how an
additional security check before retrieving a class would be helpful here.
Additionally, nothing ever stops me from calling Object#getClass if I have an
instance of an object; I may not be able to do anything with that Class
(newInstance, invoke methods, etc.), but I can call getClass.
On Sep 2, 2013, at 9:08 AM, Jörn Huxhorn wrote:
Nick also explained a performance issue in Thread#getStackTrace() and
Throwable#getStackTrace(). Keeping that in mind, I'm not sure if going for "start
small" (i.e. leaving that issue alone) would be a wise decision.
The thing is that we really need a proper solution before API freeze on
2013-10-10. This is a proper solution with regards to the API, with as little
side effects as possible (due to not changing StackTraceElement, Throwable or
Thread regarding declaringClass). I'd suggest to not restart the discussion
about whether adding this to StackTraceElement or putting it into a separate
class. We should instead try to discuss this existing API and clarify what
needs to be changed to get this approved.
I hope I don't come across rude or something. The prospect of a Java 8 without
a proper replacement API for sun.reflect.Reflection#getCallerClass is simply
giving me serious creeps and time is very pressing if we want to pull this off.
All of that: my sentiments exactly.
Nick
On Sep 2, 2013, at 10:07 AM, Remi Forax wrote:
On 09/02/2013 03:56 PM, Tom Hawtin wrote:
On 02/09/2013 09:33, Alan Bateman wrote:
From a quick scan then it appears that you've decided to ignore the
security concerns so I don't think anyone can (or should) sponsor this
patch until there is further discussion on the API and the security
implications.
I've barely looked through the patch, let alone audited it. However:
As the method appears to be mostly for tracing, performing a security check per
invocation is likely to hurt performance. Unless it was caller sensitive (which
seems to be equated with Bad Practice these days) it would also require a
doPrivileged block. Much better to perform a single security check when
acquiring a capability object, in a similar(ish) way to sun.misc.Unsafe or
SharedSecrets.
In order to work with less privileged code, it may also be worthwhile to
provide a version of the capability object parameterised with a class loader.
If code has access to a ClassLoader object, then it is consistent for it to be
given access to Classes loaded by it or child loaders, and in practice from
parent loaders (but not children of parents).
Security relating to Class objects is "interesting". The genuine security issue
with Class objects is that they have are an attenuation of the capability of their
ClassLoader. There are many ways to acquire Class objects. Some have confusing
defence-in-depth security; some, such as Object.getClass used for quick null checks, do
not.
Tom
I wonder if a java.lang.invoke.MethodHandles.Lookup is not a better object than
a ClassLoader for a capability object.
Rémi
