code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues
Alan.Bateman at oracle.com
Thu Sep 15 03:00:43 PDT 2011
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.
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.
More information about the core-libs-dev