RFR 8167546: enhance os::file_name_strncmp() on Mac OSX
Gerard Ziemski
gerard.ziemski at oracle.com
Wed Oct 10 14:21:48 UTC 2018
hi Jiangli,
> On Oct 9, 2018, at 5:10 PM, Jiangli Zhou <jiangli.zhou at oracle.com> wrote:
>
> Hi Gerard,
>
> The path string may contain more than one path component, which needs to be resolved individually, or os::Posix::realpath() would fail.
The “file_name_strncmp(char* s1, char* s2, size_t num)” API takes the “num” argument, which partitions the path string to a singular path, so it will not fail.
> Your approach using the fully resolved path is in the right direction. It will address all various path validation issues. The path validation issues occur on all platforms and need to be resolved for all. We can fix the shared SharedPathsMiscInfo::check() code, and avoid duplicating code in platform specific files. In SharedPathsMiscInfo::check() we can do a quick string comparison first as you have in bsd os::file_name_strncmp(). If the quick test fails, we can then use the canonical paths for comparison.
>
> To make the code cleaner, we could completely get rid of the separate path comparison that uses SharedPathsMiscInfo::check()/os::file_name_strncmp() and do the path sequence check as part of FileMapInfo::validate_shared_path_table(). That can be done at a later point.
It’s your code, so I defer to your decision, but just to make sure I understand what you are saying: the CDS path processing code needs
to handle this issue (8167546) as well as 8211723 (which I filed recently), so we need to make these 2 issues work for all platforms and there is some refactoring you have in mind for 8211723, which will end up fixing this issue in the process?
If that’s what you mean, then I’m perfectly fine with such approach. I provided this code, which has been extensively reviewed and tested, and which hopefully will be of help to you later.
Can you take over 8167546 (this issue) and mark it a duplicate of 8211723 (and explain to Calvin what happened)?
cheers
>
> Thanks,
>
> Jiangli
>
>
> On 10/9/18 9:30 AM, Gerard Ziemski wrote:
>> hi Ioi, Jiangli,
>>
>> After Ioi provided me with some interesting use cases I gave it some more thought, and I believe that the solution we need here is to fully resolve the paths using “realpath()” API.
>>
>> This new implementation http://cr.openjdk.java.net/~gziemski/8167546_rev4 is currently undergoing testing. I am also testing using only the “slow path” of the fix, which should increase our confidence in that code path.
>>
>> The idea is to have the filesystem (the OS) fully resolve the paths for us and return the absolute path to the destination, without us having to worry or check the filesystem capabilities, or manually complete the paths ourselves.
>>
>> For example this how the following paths will get resolved:
>>
>> (assume that "/case_insensitive_disk/foo.jar” exists)
>>
>> #1 /case_insensitive_disk/foo.jar —> /case_insensitive_disk/foo.jar
>> #2 /case_insensitive_disk/Foo.jar —> /case_insensitive_disk/foo.jar
>> #3 /case_sensitive_disk/foo.jar —> /case_sensitive_disk/foo.jar
>> #4 /case_sensitive_disk/Foo.jar —> /case_sensitive_disk/Foo.jar
>> #5 /case_insensitive_disk/../case_sensitive_disk/foo.jar —> /case_sensitive_disk/foo.jar
>> #6 /case_insensitive_disk/../case_sensitive_disk/Foo.jar —> /case_sensitive_disk/Foo.jar
>> #7 /case_sensitive_disk/../case_insensitive_disk/foo.jar —> /case_insensitive_disk/foo.jar
>> #8 /case_sensitive_disk/../case_insensitive_disk/Foo.jar —> /case_insensitive_disk/foo.jar
>> #9 ./foo.jar —> /cwd/foo.jar
>> etc.
>>
>> I believe this is a more robust and simpler solution to this problem.
>>
>> It’s also an approach that will probably fix JDK-8211723, which I filed for you the other day.
>>
>> Thank you for your patience and reviews!
>>
>>
>> cheers
>>
>>> On Oct 4, 2018, at 10:01 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>
>>> Hi Gerard,
>>>
>>> I can think of another case of mixed file systems. Assuming / is case sensitive, I think your function will treat
>>>
>>> /Volume/case_insensitive_disk/foo.jar
>>>
>>> /volume/case_insensitive_disk/foo.jar
>>>
>>> as the same file, while they are actually different.
>>>
>>> Thanks
>>>
>>> - Ioi
>>>
>>>
>>> On 10/4/18 9:56 AM, Gerard Ziemski wrote:
>>>>> On Oct 4, 2018, at 10:49 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 10/4/18 8:40 AM, Gerard Ziemski wrote:
>>>>>> Thank you for the review.
>>>>>>
>>>>>>> On Oct 3, 2018, at 7:00 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>>>
>>>>>>> Hi Gerard,
>>>>>>>
>>>>>>> I don't know much about Mac OS, so I am just commenting from a C/C++ perspective:
>>>>>>>
>>>>>>> 39 #include <unistd.h>
>>>>>>>
>>>>>>> This file has already been included above.
>>>>>> I missed that, fixed.
>>>>>>
>>>>>>
>>>>>>> 46 if (path[0] != '/') {
>>>>>>> 47 // The path is relative, so use the current working directory
>>>>>>> 48 char cwd[PATH_MAX];
>>>>>>> 49 os::get_current_directory(cwd, PATH_MAX);
>>>>>>> 50 return (pathconf(cwd, _PC_CASE_SENSITIVE) != 1);
>>>>>>> 51 }
>>>>>>>
>>>>>>>
>>>>>>> https://lists.apple.com/archives/darwin-dev/2007/Apr/msg00036.html suggests that pathconf is not reliable on NFS, SMBFS, etc.
>>>>>>>
>>>>>>> Also, what happens if path is a symbolic link that spans across two file systems, where one of them is case sensitive and the other is not?
>>>>>> I tested symbolic links (on Mac) out of curiosity before and they just don’t work. Whether CDS wants to start supporting them or not is outside the scope here.
>>>>>>
>>>>> Hi Gerard,
>>>>>
>>>>> Could you provide more details? Maybe file a bug?
>>>> Filed https://bugs.openjdk.java.net/browse/JDK-8211723, there are other corner cases I listed there that do not work.
>>>>
>>>>
>>>>>>> I think is_filesystem_case_insensitive could potentially return a false positive, which would cause two "unequal" file names to appear "equal". This would defeat the pathname validation in CDS.
>>>>>> The false positive can be fixed by us explicitly checking for pathconf(_PC_CASE_SENSITIVE) returning 0, so the line 50 (and the other case) should be:
>>>>>>
>>>>>> 50 return (pathconf(cwd, _PC_CASE_SENSITIVE) == 0);
>>>>>>
>>>>>> That means we check using case insensitive comparison only if we are explicitly told by “pathconf” that the underlying file system does not support case sensitive names. If it does, then “pathconf” will return 1, and we know for sure that it’s case sensitive. In all other cases (like on NFS, SMBFS) it returns “-1” and we assume it’s case sensitive too (which may or may not lead to false negative, but is what we do now).
>>>>>>
>>>>> That sounds safer.
>>>>>
>>>>> For the relative path, though, I think we still have a problem:
>>>>>
>>>>> 46 if (path[0] != '/') {
>>>>> 47 // The path is relative, so use the current working directory
>>>>> 48 char cwd[PATH_MAX];
>>>>> 49 os::get_current_directory(cwd, PATH_MAX);
>>>>>
>>>>>
>>>>> If your CWD is /Volumes/case_insensitive_disk/foo
>>>>>
>>>>> and the path is ../../case_sensitive_disk/bar
>>>>>
>>>>> Wouldn't you get an incorrect answer?
>>>>>
>>>>> Another case is, when / is case insensitive
>>>>>
>>>>> CDW = /
>>>>> path is Volumes/case_sensitive_disk/foo
>>>> Interesting corner cases - fixed by turning a relative path into a full path using CWD - please see rev3.
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8167546
>>>> webrev: http://cr.openjdk.java.net/~gziemski/8167546_rev3
>>>> testing: Mach5 hs-tier1,2,3,4,5,6,7 (in progress…)
>>>>
>>>>
>>>> cheers
>>>>
>>>>
>
More information about the hotspot-runtime-dev
mailing list