[rfc][icedtea-web] policytool in itweb-settings

[Andrew Hughes]

----- Original Message -----
> On 01/22/2014 07:57 PM, Andrew Azores wrote:
> > On 01/22/2014 07:26 AM, Jacob Wisor wrote:
> >> On 01/22/2014 01:30 AM, Jacob Wisor wrote:
> >>> On 01/20/2014 09:11 PM, Andrew Azores wrote:
> >>>> On 01/20/2014 02:50 PM, Andrew Azores wrote:
> >>>>> On 01/20/2014 07:27 AM, Jiri Vanek wrote:
> >>>>>> On 01/17/2014 10:08 PM, Andrew Azores wrote:
> >>>>>>> (snip)
> >>>>>>
> >>>>>> Ouch. See my reply to patch.
> >>>>>
> >>>>> Didn't see any mail, but we discussed on IRC. Here's a recap:
> >>>>> - The reflection hack is horrible. It doesn't quite work, and it's
> >>>>> super ugly.
> >>>>> - The process not exiting is definitely a problem. We should not be
> >>>>> playing
> >>>>> around with reflection on the policytool like this.
> >>>>> - Calling PolicyTool.main() directly breaks the automated build(?). We
> >>>>> don't
> >>>>> want to make it a build dep, and it's too minor to bother with a
> >>>>> configure
> >>>>> step/flag(?).
> >>>>> - Solution: Runtime#exec() makes its return, with a fallback method of
> >>>>> reflectively invoking PolicyTool.main() if for any reason the exec
> >>>>> fails
> >>>>> (non-POSIX system, executable not on PATH, etc)
> >>>>>
> >>>>> So here's a patch implementing this solution, with lots of extra added
> >>>>> documentation.
> >>>>>
> >>>>
> >>>> And this time with better respect for event dispatch thread, I hope.
> >>>> (thanks
> >>>> omajid)
> >>>
> >>> +    private static void policyToolLaunchHelper(final JFrame frame, final
> >>> String
> >>> filePath) {
> >>> +        final String[] launchCommand = new String[] { "policytool",
> >>> "-file",
> >>> filePath };
> >>>
> >>> Ah, the caveats of anonymous classes and threads again: The launchCommand
> >>> local
> >>> constant is not guaranteed to be available when Runnable.run() is
> >>> scheduled
> >>> to run.
> >>>
> >>> +        new Thread(new Runnable() {
> >>> +            @Override
> >>> +            public void run() {
> >>> +                try {
> >>> +                    final Process ptool =
> >>> Runtime.getRuntime().exec(launchCommand);
> >>>
> >>> The launchCommand local constant referenced here which lives on the stack
> >>> of the
> >>> anonymous class's enclosing policyToolLaunchHelper() method is not
> >>> guaranteed to
> >>> be valid or to exist after the thread had been started.
> >>> policyToolLaunchHelper()
> >>> may terminate and thus its local stack be cleaned up /before/
> >>> Runnable.run() is
> >>> scheduled to run, leaving no constant for reference. I am not entirely
> >>> sure
> >>> about what the Java Programming Language Spec and/or JVM Spec define in
> >>> this
> >>> case but I would bet that it has intentionally been left undefined. There
> >>> is
> >>> certainly no problem when anonymous class's methods and their enclosing
> >>> methods
> >>> share local variables running on the same thread. This case has been
> >>> defined.
> >>> But, in the case of threads, I would be rather careful. Sorry, I do not
> >>> have the
> >>> time to look it up in the specs. :-/
> >>
> >> Well, I did look it up because it was quite an intriguing question.
> >> Sub-paragraph 3 of Java Language Specification §17.4.1 says it:
> >> "Local variables (§14.4), formal method parameters (§8.4.1), and exception
> >> handler parameters (§14.20) are never shared between threads and are
> >> unaffected by the memory model." ;-)
> >>
> >>> To get thread-safe, you would have to pass launchCommand as a parameter
> >>> to
> >>> Runnable's constructor and store its reference in a member variable.
> >>>
> >>> Btw, the preferred way to construct Process objects since JDK 1.5 is to
> >>> use
> >>> ProcessBuilder. ProcessBuilder would also allow you to set policytool's
> >>> current
> >>> working directory to user.home, which is advisable.
> >>> [...]
> >
> > Amazing, thank you for all the help with this! But are you sure about this
> > problem of the constant going out of scope of the Thread? Yes, variables,
> > method
> > parameters, and exception handler parameters will never be *shared* between
> > threads (as in each Thread will have a local variable by the same name but
> > these
> > two will be distinct from each other in memory), but does this really mean
> > that
> > the variable can go out of scope for the Thread by the time it runs?
> 
> First of all, the term "scope" in the context of programming languages only
> refers to visibility of memory resource identifiers from a given section of
> source code. It has no implications on nor any assumptions can be made based
> upon it about the visibility of memory resources in any target execution
> environment. A given programming language scope may or may not have an
> equivalent abstraction, representation, or constraint in the target execution
> environment. In Java, both cases can be found, depending on the language
> construct. So, please do not confuse applying the term "scope" from a
> programming language to an execution environment. Most execution environments
> do
> not deal with scopes. They rather have limitations on addressing modes or
> have
> memory management units etc.
> 
> In the source code at hand, Runnable.run() does not lose scope of
> launchCommand.
> Otherwise, the source code would not compile, simply because scope checking
> is
> done at compile-time, never at run-time. However, what bothered me was that
> Runnable.run() needs to access the object via the reference stored in the
> local
> constant variable launchCommand. Accesses can only be accomplished through
> reading the value of a field, an array element, or a local variable (or a
> return
> value that is also placed on the stack). And, since launchCommand isn't
> passed
> and stored nowhere in the source code explicitly to Runnable, I would rather
> assume it is not going to be available at run-time. So, I decided to dig
> deeper
> into this by disassembling the relevant classes. As it turns out, the OpenJDK
> 7
> compiler synthesizes a "final synthetic java.lang.String[] val$launchCommand"
> field in Runnable. IMHO this is a violation of the Java Language Spec. It is
> no
> violation of the JVM Spec, of course. But, it is annoying and confusing that
> the
> so called "Java reference implementation" silently starts to violate the Java
> Language Spec in order to get around corner cases or to patch things up so
> that
> it just runs. :-( This means that you can never be sure about the code that
> the
> compiler is going to generate or rather you have to be always aware that it
> will
> possibly generate some wired and dubious stuff into your byte code. This is
> totally against Java's original intention of a compile-time error preventing
> type-safe verifiable platform. *rant start* They have turned this idea upside
> down by eliminating errors silently or implicitly instead of leaving
> programmers
> in control of their programs and the way they want to solve problems. Why did
> they do that? Did they fear some programmers being to dump to solve problems
> themselves? *end of rant*
> 

I'm assuming this is using OpenJDK's javac to compile?  Out of interest, have you
tried compiling it with ecj and seeing if that gives a different result or emulates
the same hack?  I suspect this is something that was done to support inner classes.

As to the PolicyTool issue itself, it seems the bug in building on 6 is due to:

changeset:   2400:b3466e2c3819
user:        mchung
date:        Tue May 18 13:12:46 2010 -0700
summary:     6951599: Rename package of security tools for modularization

a change in OpenJDK 7 which moved PolicyTool from sun.security.tools to its own
package, sun.security.tools.PolicyTool:

-       $(call make-launcher, policytool, sun.security.tools.PolicyTool, , )
+       $(call make-launcher, policytool, sun.security.tools.policytool.PolicyTool, , )

If that's the only issue with using that, I don't see why we couldn't just backport
that change to OpenJDK 6 so the location is consistent across all JDKs again.
-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07