Re: code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues
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.
RE: code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues
Hi, I'm not a formal reviewer, but I've implemented this same thing for IKVM.NET which has to simulate the behavior of CreateProcess when running on Windows (because it uses the .NET API to start a process and that doesn't have this hack). Your algorithm doesn't match CreateProcess, because that also takes into account various directories to search (the application's directory, the current directory, the system directory, the windows directory and the PATH). Regards, Jeroen From: core-libs-dev-boun...@openjdk.java.net [core-libs-dev-boun...@openjdk.java.net] on behalf of Michael McMahon [michael.x.mcma...@oracle.com] Sent: Tuesday, September 13, 2011 6:17 PM To: core-libs-dev@openjdk.java.net Subject: code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues Can I get the following webrev reviewed please? http://cr.openjdk.java.net/~michaelm/7051946/webrev.1/ The problem is when calling Runtime.exec(String) with a program name containing white space (on win32), it is difficult to distinguish between the program name and any parameters to it. Eg. C:\A B\C D\E foo bar. Does this string represent the program name or are foo and bar arguments to a program called E? And there are many other possibilities. We just pass the whole string to windows and it does an ok job of disambiguating according to a defined algorithm. There are two problems however: 1) our security check doesn't do exactly the same thing as windows. So we may end up checking for a different file to what gets executed. 2) when the file doesn't exist, the error returned is truncated. In the example above, it would think C:\A is the non-existing program. The problem doesn't occur on Solaris/Linux because those OSes never try to disambiguate the way windows does. So, there is currently already consistency between the security check and the path to be run. Effectively, this way of calling Runtime.exec() never worked on those platforms and you always had to use one of the other multi-arg methods. So, the solution is first to refactor ProcessBuilder and ProcessImpl, by moving the generation of the exception down to ProcessImpl (when the file is not found) and also to move the security check down to ProcessImpl, where we can do the windows specific checking, and for Solaris and Windows there's no change in behavior beyond that. Thanks, Michael.
Re: code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues
On 13/09/11 18:26, Alan Bateman wrote: Michael McMahon wrote: Can I get the following webrev reviewed please? http://cr.openjdk.java.net/~michaelm/7051946/webrev.1/ I scanned this briefly (I haven't done a detailed code review) but one initial comment is that Runtime.exec will now throw IllegalArgumenetException for that a case that isn't specified. The current javadoc says it is only thrown If command is empty and maybe this needs to be re-examined. I assume such cases would cause IOException to be thrown today. -Alan. 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. - Michael.
Re: code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues
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.
Re: code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues
Michael McMahon wrote: Can I get the following webrev reviewed please? http://cr.openjdk.java.net/~michaelm/7051946/webrev.1/ I scanned this briefly (I haven't done a detailed code review) but one initial comment is that Runtime.exec will now throw IllegalArgumenetException for that a case that isn't specified. The current javadoc says it is only thrown If command is empty and maybe this needs to be re-examined. I assume such cases would cause IOException to be thrown today. -Alan.
Re: code review request: 7051946: Runtime.exec(String command) / ProcessBuilder command parsing issues
On 14/09/2011 3:26 AM, Alan Bateman wrote: Michael McMahon wrote: Can I get the following webrev reviewed please? http://cr.openjdk.java.net/~michaelm/7051946/webrev.1/ I scanned this briefly (I haven't done a detailed code review) but one initial comment is that Runtime.exec will now throw IllegalArgumenetException for that a case that isn't specified. The current javadoc says it is only thrown If command is empty and maybe this needs to be re-examined. I assume such cases would cause IOException to be thrown today. Aside: ProcessBuilder.start seems to have a number of undocumented failure modes compared to Runtime.exec I can't comment on the details of the windows logic, but the basic code refactoring seems okay to me. David -Alan.
