Review request: JDK-8012453 (process) Runtime.exec(String) fails if command contains spaces [win]
alexey.utkin at oracle.com
Thu Apr 25 14:41:55 UTC 2013
Thanks for your comments.
I did the changes in code.
Please, read my answers below.
Here is fixed version:
On 24.04.2013 18:23, Alan Bateman wrote:
> On 24/04/2013 13:58, Alexey Utkin wrote:
>> I changed in the ProcessBuilder class to restore the compatibility
>> with Java documentation. In accordance with spec, the
>> IllegalArgumentException exception could not be thrown from the start
>> method. I made it a cause for declared IOException.
> This part looks good to me.
>> Thanks for your attention. The test was extended to cover the case.
>> New version:
>> Lines 381-382 in ProcessImpl.java file are responsible for the second
>> call to Security Manager.
>> The call with [".\Program Files\doNot.cmd" arg] param does not pass
>> the second Security Manager verification in the ExecCommand.java test.
> Overall I think this looks okay.
> In getTokensFromCommand then I guess I would have used a regex rather
> than legacy StringTokenizer (I realize Runtime is specified to use
> StringTokenizer for the initial split). I think I agree with Martin on
> wondering why LinkedList is used but it's not an issue as this is the
> fallback. Minor nit is that the brace on L161 can be moved to the end
> of the previous line.
> A minor comment is that -Djdk.lang.Process.allowAmbigousCommands=false
> will not do what is intended (the value of the property is not examined).
> The test needs a copyright header but otherwise looks comprehensive.
> One thing I see is that it doesn't read from the child's output/error
> streams so if there is a problem then it will be hard to diagnose from
> the logs.
The diagnostic includes logging of the command line in problem. It
should not be a problem to localize issue.
> Minor comments on the test is that the creation of the commands could
> use try-with-resources.
More information about the core-libs-dev