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

[Andrew Azores]

On 01/23/2014 05:16 PM, Andrew Hughes wrote:
>
> ----- 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.

I don't think it's worth backporting this, at least not if it's just to 
benefit IcedTea-Web. I wasn't aware of this, but all it means is making 
the reflective fallback launch check for PolicyTool in one more package. 
Still really not ideal, but it's a very small thing to add compared to 
the rest of the work that's developed around getting PolicyTool hooked 
into the control panel here. Certainly not worth risking breaking other 
applications that might depend on PolicyTool's location being fixed in 6 
(although I suspect that number is *incredibly* small). If it does get 
backported then great, the current patch is sufficient, but I'm going to 
say that this use case shouldn't be regarded as significant enough to 
influence the backport decision.

I suppose this means tomorrow I'll post another patch that actually has 
PolicyPanel check for the PolicyTool in both locations. I'll be so glad 
when we have a nice replacement for PolicyTool and all of this 
ProcessBuilder and fallback reflective launch can just go away.

Thanks,

-- 
Andrew A