code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues

[Alan Bateman]

Alan Bateman wrote:
> Michael McMahon wrote:
>> We could add a sentence to the existing  @throws clause for IAE.
>>
>> So it would say "If |command| is empty, or on some platforms, may be 
>> thrown if command contains illegal characters".
>>
>> I'm reluctant to be more precise than that, since as far as I know, 
>> there doesn't appear to be
>> any illegal characters for filenames in Unix.
> I agree that you can't be too specific. I'm not sure if "illegal 
> characters" should be in the wording as it begs the question as to 
> which characters are legal. An alternative to trying to come up with 
> some wording is to just specify the long standing behavior which seems 
> to be that IOException is thrown if an I/O errors or the command line 
> is malformed. I see David's reply pointing out that there are similar 
> issues with ProcessBuilder. That is only specified to throw 
> IndexOutOfBoundException if the command line is empty so it would be 
> good to check those cases too.
>
> -Alan.
I went through the rest of the changes.

In src/solaris/classes/java/lang/ProcessImpl.java then the permission 
check needs to happen before the streams are redirected as otherwise it 
would allow an attacker to truncate arbitrary files. I think the 
alignment is a bit out at L132-142.

In src/windows/classes/java/lang/ProcessImpl.java L211-216 is using 
toLowerCase (which is locale specific) and is assuming that prog and 
lprog will be the same length.

The algorithm in getProgramPath seems okay although I think it can be 
cleaned up a bit (coding style is inconsistent with the rest of the file 
for example, is the StringBuilder actually needed). Minor comment is 
that "fileHasNoDot" is an odd method name, maybe hasDot would be nicer. 
There's space between File and "(" at line 323.

A general comment is that what would it take to move this code away from 
using StringTokenizer? I realize Runtime.exec has javadoc to say that it 
uses StringTokenizer and I just wonder if that can be re-visited.

-Alan.