Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
On 05/03/2013 01:08 AM, Dan Xu wrote: Hi All, Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered. As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review! webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/ I like this approach, thanks. I think the additional parenthesis around the return expression and the ternary operator are not part of the usual coding standard. -- Florian Weimer / Red Hat Product Security Team
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
On 05/06/2013 06:59 AM, Florian Weimer wrote: On 05/03/2013 01:08 AM, Dan Xu wrote: Hi All, Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered. As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review! webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/ I like this approach, thanks. I think the additional parenthesis around the return expression and the ternary operator are not part of the usual coding standard. Thank you for your good review. I will fix the format. As for the possible scenario, NUL appearing at the end of a path, I will regard it as the same kind of invalidity as other embedded NUL cases since there are no obvious use cases for that. I will push the fix today. Thanks! -Dan
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
On 03/05/2013 00:08, Dan Xu wrote: Hi All, Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered. As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review! webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/ -Dan This looks much better. I guess a Boolean would have worked as well as adding the PathStatus enum but what you have seems okay. It would be good to get Sherman's confirmation that we don't need to be concerned about anything else encoding to include NUL. -Alan.
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
On 5/3/13 6:48 AM, Alan Bateman wrote: On 03/05/2013 00:08, Dan Xu wrote: Hi All, Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered. As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review! webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/ -Dan This looks much better. I guess a Boolean would have worked as well as adding the PathStatus enum but what you have seems okay. It would be good to get Sherman's confirmation that we don't need to be concerned about anything else encoding to include NUL. -Alan. The change looks fine. I don't see any encoding issue, though this reminds me one possible use scenario. Do we want to make it path valid, if the NUL is at the end of the path? This should not been an issue normally though, everything from the native does not have it. Just wonder if this scenario might work before (?), with no potential vuln risk (?), we may want to keep it? No, I don't know any real code has the NUL attached to the end, just a wild guess. It's definitely fine to wait for any complain, if there will be, and then react. -Sherman
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
Hi All, Thanks for all your comments. Based on the previous feedback, I have moved to the other approach, i.e., to fail file operations if the invalid NUL characher is found in a file path. As you know, due to the compatibility issue, we cannot throw an exception immediately in the File constructors. So the failure is delayed and only shown up when any file operation is triggered. As for FileInputStream, FileOutputStream, and RandomAccessFile classes, the FileNotFoundException will be thrown right away since their spec allow this exception happen in the constructors. Thanks for your review! webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/ -Dan
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
On 02/27/2013 03:52 AM, David Holmes wrote: On 27/02/2013 12:31 PM, Dan Xu wrote: Thank you, Mike. The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found. That still gives the choice of deleting the nul characters, or treating them as a terminator. And do we really want/need to maintain compatability in this case anyway? What reasonable expectations can anyone have if they have embedded nuls? What does a FileInputStream for example do when trying to open a File with embedded NUL chars on UNIX/Windows ? Does it try to open a truncated path? If so, then perhaps normalize could do that beforehand... Regards, Peter David -Dan On 02/26/2013 04:33 PM, Mike Duigou wrote: Hi Dan; External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992 I would like to better understand why silently removing the garbage null characters is the right answer here. If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled. Mike On Feb 26 2013, at 16:10 , Dan Xu wrote: Hi All, Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly. Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks! Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/ -Dan
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
Ouch. That is unfortunate that File cannot reject bad input. Perhaps FileInputStream etc. should throw a specialized Bad Filename FnF for paths containing NUL if the underlying filesystem does not support NUL? Masking garbage input always really scares me. Mike On Feb 27 2013, at 02:40 , Alan Bateman wrote: On 27/02/2013 02:31, Dan Xu wrote: Thank you, Mike. The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found. -Dan Right, we can't change the constructor to throw an exception, particularly if this fix is going to be back-ported to 7u. For NIO then it's not an issue because getPath was specified from the begining to throw the unexpected InvalidPathException when it is given garbage. -Alan.
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
Because we cannot change the behaviour of File constructorsto error out the bad input immediately, the changes will be scattered all over many java io methods, especially methods in File.java, if we choose to reject bad inputs. And due to the delay of the rejection, it also brings a little headache to developers to find out why the exception happens.Comparing with that, the proposed approach is simple and resilient. In addition, the currentnormalization logic already tolerates some invalid path-name formats and invalid characters. This approach can be regarded as a extension of the tolerance to theNUL character. It is consistent with what the native platform would do with a null and what/how we are currently normalizing the slash.Thanks! -Dan On 02/27/2013 10:31 AM, Mike Duigou wrote: Ouch. That is unfortunate that File cannot reject bad input. Perhaps FileInputStream etc. should throw a specialized Bad Filename FnF for paths containing NUL if the underlying filesystem does not support NUL? Masking garbage input always really scares me. Mike On Feb 27 2013, at 02:40 , Alan Bateman wrote: On 27/02/2013 02:31, Dan Xu wrote: Thank you, Mike. The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found. -Dan Right, we can't change the constructor to throw an exception, particularly if this fix is going to be back-ported to 7u. For NIO then it's not an issue because getPath was specified from the begining to throw the unexpected InvalidPathException when it is given garbage. -Alan.
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
Hi Dan; External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992 I would like to better understand why silently removing the garbage null characters is the right answer here. If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled. Mike On Feb 26 2013, at 16:10 , Dan Xu wrote: Hi All, Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly. Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks! Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/ -Dan
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
Filenames are a lot like environment variables, and we already have code
that rejects NUL chars in environment variable names:
// Check that name is suitable for insertion into Environment map
private static void validateVariable(String name) {
if (name.indexOf('=') != -1 ||
name.indexOf('\u') != -1)
throw new IllegalArgumentException
(Invalid environment variable name: \ + name + \);
}
On Tue, Feb 26, 2013 at 4:33 PM, Mike Duigou mike.dui...@oracle.com wrote:
Hi Dan;
External link to the bug (will hopefully work soon):
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992
I would like to better understand why silently removing the garbage null
characters is the right answer here.
If null is an invalid character for the underlying operating system or
filesystem then perhaps an error should be signalled.
Mike
On Feb 26 2013, at 16:10 , Dan Xu wrote:
Hi All,
Please help review the fix for JDK-8003992: File and other classes in
java.io do not handle embedded nulls properly.
Java IO, not like NIO, does not check NUL character in the given file
path name, which brings confusion to Java users. This fix is going to
address this issue by removing any NUL character from the given file path.
And it also cleans and optimizes the path-name normalization logic. Thanks!
Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992
webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/
-Dan
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
Thank you, Mike. The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found. -Dan On 02/26/2013 04:33 PM, Mike Duigou wrote: Hi Dan; External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992 I would like to better understand why silently removing the garbage null characters is the right answer here. If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled. Mike On Feb 26 2013, at 16:10 , Dan Xu wrote: Hi All, Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly. Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks! Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/ -Dan
Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly
On 27/02/2013 12:31 PM, Dan Xu wrote: Thank you, Mike. The reason not to throw out an exception is for the backward compatibility. Due to that, the constructorof File object with NUL willnever fail.While in NIO, it is defined in the spec to throw out exceptions when invalid NUL character is found. That still gives the choice of deleting the nul characters, or treating them as a terminator. And do we really want/need to maintain compatability in this case anyway? What reasonable expectations can anyone have if they have embedded nuls? David -Dan On 02/26/2013 04:33 PM, Mike Duigou wrote: Hi Dan; External link to the bug (will hopefully work soon): http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003992 I would like to better understand why silently removing the garbage null characters is the right answer here. If null is an invalid character for the underlying operating system or filesystem then perhaps an error should be signalled. Mike On Feb 26 2013, at 16:10 , Dan Xu wrote: Hi All, Please help review the fix for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly. Java IO, not like NIO, does not check NUL character in the given file path name, which brings confusion to Java users. This fix is going to address this issue by removing any NUL character from the given file path. And it also cleans and optimizes the path-name normalization logic. Thanks! Bug: https://jbs.oracle.com/bugs/browse/JDK-8003992 webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.00/ -Dan
