RFR: 8194154: JDK crashes parsing path string contains '//' on linux

[yumin qi]

Hi, Alan

  The real reason, for why File.getCanonicalPath() will fail after
"user.dir" property changed is, in File.java:

     public String getCanonicalPath() throws IOException {
        if (isInvalid()) {
            throw new IOException("Invalid file path");
        }
        return fs.canonicalize(fs.resolve(this));
    }

    It passed this File to FileSystem.resolve(File):

    public String resolve(File f) {
        if (isAbsolute(f)) return f.getPath();
        return resolve(System.getProperty("user.dir"), f.getPath());
    }
    Note, here it get property of "user.dir" and passed the string directly
into resolve(String, String). Checked all the usage of this function in all
other places, they all normalize the string first then pass it to resolve.
Like:

    File.java:

        public File(String parent, String child) {
        if (child == null) {
            throw new NullPointerException();
        }
        if (parent != null) {
            if (parent.equals("")) {
                this.path = fs.resolve(fs.getDefaultParent(),
                                       fs.normalize(child));
            } else {
                this.path = fs.resolve(fs.normalize(parent),
                                       fs.normalize(child));
            }
        } else {
            this.path = fs.normalize(child);
        }
        this.prefixLength = fs.prefixLength(this.path);
    }

    Since the property string contains non-normalized characters, it
crashed in native canonicalize.
    I believe user.dir from the system is normalized, so it is OK but after
it is changed like "/home/a/b/c/", it crashed.

    Now with using cached "user.dir", the problem is gone.
    The ultimate guaranteed solution may be:

    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 = normalize(props.getProperty("user.dir"));     // is it
necessary?
    }

     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());
     }


    So the changes in resolve should be removed.

    Since the bug is talking about the crash, the real reason is user.dir
should not be changed, how about changing description to
    8194154: System property user.dir should not be changed.

     The test case renamed to: UserDirChangedTest.java   ?

Thanks
Yumin



On Thu, Feb 15, 2018 at 10:41 AM, yumin qi <yumin.qi at gmail.com> wrote:

> There are two problems here, so become complex.
> 1) crash on parsing "//", which included in file path, on linux. This is
> fixed in UnixFileSystem.java in resolve function.
> 2) user.dir should be read only. This is fixed in both UnixFileSystem.java
> and WinNTFileSystem.java.
>
> The test case covers two of them.
> Should we handle them in two separate bugs?
>
> Yumin
>
>
> On Thu, Feb 15, 2018 at 10:00 AM, yumin qi <yumin.qi at gmail.com> wrote:
>
>> OK, let's work on a suitable test case.
>>
>> Thanks
>> Yumin
>>
>> On Thu, Feb 15, 2018 at 9:41 AM, Alan Bateman <Alan.Bateman at oracle.com>
>> wrote:
>>
>>> On 15/02/2018 17:25, yumin qi wrote:
>>>
>>>> Alan,
>>>>
>>>>   The real reason is if we do not change on resolve, the string passed
>>>> into native canonicalize will be like "//./a/" which is not expected in
>>>> native part. In native part, we resume that no more "//" in the path string
>>>> (already normalized in java).
>>>>
>>>> If that is the case then the test doesn't match the issue we have been
>>> discussing on this thread. Would it be possible to create a test case for
>>> the issue that you are actually trying to address?
>>>
>>> -Alan
>>>
>>
>>
>