RFR: 8194154 patch for crash at File.getCanonicalPath()

[Roger Riggs]

Hi,

In the test, the message can be improved; though the exception should 
never occur
it will be misleading if it does.
Instead:
     "Changing property user.dir should have no effect on the 
getCanonicalPath"

It is cleaner to throw AssertionError for the test failure.
Throwing FileSystemException makes it look like it came from inside the 
file system implementation.

Regards, Roger

On 2/13/2018 6:45 PM, yumin qi wrote:
> Alan,
>
>    In fact, if property "user.dir" is cached, the problem will go away
> without any changes to native or UnixFileSystem.java, since it won't
> canonicalize the string supplied from user. The output will be
>    getProperty("user.dir") + "/" + file. (The result is not as expected,
> user can not change the property, means user has to work around the problem
> to reach their targeted directory to do things like loading classes)
>
>    Any change to "user.dir" in java program does not have any effect.
>    For Windows:
>
> --- a/src/java.base/windows/classes/java/io/WinNTFileSystem.java Fri Feb 02
> 10:32:59 2018 -0800
> +++ b/src/java.base/windows/classes/java/io/WinNTFileSystem.java Tue Feb 13
> 23:40:21 2018 +0000
> @@ -43,12 +43,14 @@
>       private final char slash;
>       private final char altSlash;
>       private final char semicolon;
> +    private final String userDir;
>
>       public WinNTFileSystem() {
>           Properties props = GetPropertyAction.privilegedGetProperties();
>           slash = props.getProperty("file.separator").charAt(0);
>           semicolon = props.getProperty("path.separator").charAt(0);
>           altSlash = (this.slash == '\\') ? '/' : '\\';
> +        userDir = props.getProperty("user.dir");
>       }
>
>       private boolean isSlash(char c) {
> @@ -347,7 +349,7 @@
>       private String getUserPath() {
>           /* For both compatibility and security,
>              we must look this up every time */
> -        return normalize(System.getProperty("user.dir"));
> +        return normalize(userDir);
>       }
>
>       private String getDrive(String path) {
>
>    Does it need normalize(userDir) in getUserPath()? I think we need here.
>
>    I will update the webrev and send for another round of review: the change
> in native will be reverted, and made changes to UnixFileSystem.java.
>
>
> Thanks
> Yumin
>
>
> On Mon, Feb 12, 2018 at 3:09 AM, Alan Bateman <Alan.Bateman at oracle.com>
> wrote:
>
>> On 09/02/2018 18:01, Alan Bateman wrote:
>>
>>> :
>>>
>>> I'll study the patch you have but I think we also need to create issues
>>> to get us to the point where changing this system property in a running VM
>>> doesn't impact running code.
>>>
>> Looking at it again, I think we should change java.io.UnixFileSystem (and
>> the Windows equivalent) to cache the value of user.dir to avoid difficult
>> to diagnose issues with bad code changing the value of this property in a
>> running VM. This should reduce the issue down to cases where user.dir is
>> changed on the command line (never supported either of course) to a value
>> that is not "/" but has trailing or duplicate slashes.
>>
>> When reduced down then the alternatives are to change the native
>> canonicalize method as you have done or alternatively do it once at
>> UnixFileSystem initialization time so that canonicalize does not have to
>> deal with this case. The former would require changing the description of
>> the function (it currently reads "The input path is assumed to contain no
>> duplicate slashes"), the latter avoids any changes to the native
>> implementation.
>>
>> -Alan
>>
>>
>> diff -r 0937e5f799df src/java.base/unix/classes/jav
>> a/io/UnixFileSystem.java
>> --- a/src/java.base/unix/classes/java/io/UnixFileSystem.java    Sat Feb
>> 10 07:06:16 2018 -0500
>> +++ b/src/java.base/unix/classes/java/io/UnixFileSystem.java    Mon Feb
>> 12 10:49:40 2018 +0000
>> @@ -34,12 +34,14 @@
>>       private final char slash;
>>       private final char colon;
>>       private final String javaHome;
>> +    private final String userDir;
>>
>>       public UnixFileSystem() {
>>           Properties props = GetPropertyAction.privilegedGetProperties();
>>           slash = props.getProperty("file.separator").charAt(0);
>>           colon = props.getProperty("path.separator").charAt(0);
>>           javaHome = props.getProperty("java.home");
>> +        userDir = props.getProperty("user.dir");
>>       }
>>
>>
>> @@ -128,7 +130,11 @@
>>
>>       public String resolve(File f) {
>>           if (isAbsolute(f)) return f.getPath();
>> -        return resolve(System.getProperty("user.dir"), f.getPath());
>> +        SecurityManager sm = System.getSecurityManager();
>> +        if (sm != null) {
>> +            sm.checkPropertyAccess("user.dir");
>> +        }
>> +        return resolve(userDir, f.getPath());
>>       }
>>
>>       // Caches for canonicalization results to improve startup performance.
>>
>>