RFR (M) 8191521: handle long relative path specified in -Xbootclasspath/a on windows

[Calvin Cheung]

Hi Ralf,

Thanks for working on this RFE. The changes look good in general and 
other functions such as os::stat() os::open() look cleaner now.

os_windows.cpp

4315       } else {
4316         prefix = L"\\\\?\\UNC";
4317         prefix_off = 1; // Overwrite the first char with the prefix.
4318       }

Do you mean if the original path is something like "\\\\x\\y", it would 
become the following?

     ""\\\\?\\UNC\\x\\y"

Maybe add an example in the comment?

4368   if (err != ERROR_SUCCESS) {
4369     os::free(result);

I think you need a NULL check on result before calling os::free() 
because result could be NULL if the os::malloc() call at line 4328 has 
failed.

Some minor nit in the comment of the wide_abs_unc_path() function:

line 4282 please add a blank space before "The"

line 4283 "er" should be "err"

Could you also mention that the function is based on pathToNTPath() in 
io_util_md.cpp?

test_os_windows.cpp

I haven't reviewed this file in details but I have tried your patch and 
saw failures in tier1 testing.

Excerpt from the log file (I can send you the entire log off list if you 
like):

t:/workspace/open/test/hotspot/gtest/runtime/test_os_windows.cpp(276): 
error: Expected: (fd) != (-1), actual: -1 vs -1
os::open failed for 
"\\\\localhost\\T$\\testoutput\\test-support\\jtreg_open_test_hotspot_jtreg_tier1_common\\scratch\\1\\os_windows_long_paths_dir_LLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLL\\not_empty_directory_with_long_path\\file" 
with errno 67
t:/workspace/open/test/hotspot/gtest/runtime/test_os_windows.cpp(273): 
error: Expected equality of these values:
   os::stat(buf, &st)
     Which is: -1
   0
os::stat failed for 
"\\\\localhost\\T$\\testoutput\\test-support\\jtreg_open_test_hotspot_jtreg_tier1_common\\scratch\\1\\os_windows_long_paths_dir_LLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLL\\not_empty_directory_with_long_path\\file"

LongBCP.java

This one looks good.

thanks,

Calvin

On 7/4/19 3:56 AM, Schmelter, Ralf wrote:
> Hi,
>
> can you please review this patch to fix various long path related problems in the hotspot os code on Windows.
>
> As described in the bug the current code cannot handle relative paths in these cases:
>
>    1.  If the relative path is < 260 chars, but the absolute path is > 260 chars. In this case if the I/O method uses the *A variant of the system call as an optimization, it will fail.
>    2.  If the relative path is > 260 chars or the I/O method always uses the *W variant. In this case the create_unc_path() method is called, which just prepends \\?\ to the relative path. But this is not a valid path to use and the system call will fail.
>
> Additionally there are problems with some other kinds of paths:
>
>    1.  An absolute path which contains '.' or '..' parts and is > 260 chars or the I/O method always uses the *W variant. When given to the create_unc_path() method, it will just prepend \\?\. But this is not a valid path to use and the system call will fail.
>    2.  An UNC path which is > 260 or the I/O method always uses the *W variant. The create_unc_path erroneously converts \\host\path to \\?\UNC\\host\path<file://%3f/UNC/host/path> (notice the double backslash before the host name). This again is not a valid path. Additionally '.' or '..' parts would not be handled correctly too.
>
> To fix this I've introduced a new function, which converts a path to a wide character unc path, calling _wfullpath() to make the path absolute if needed and to remove the '.' and '..' path parts. I've adjusted all methods which used create_unc_path() to use the new method. And I removed all fallback code using the ANSI variants, since benchmarking showed that on my machine the additional overhead of converting to a wchar and potentially calling _wfullpath() was less than 5% of the actual I/O routine called. And for this reason, why I haven't tried to optimize avoiding calls to _wfullpath() (e.g. checking for '.' and '..' and only calling it if we find this in the path).
>
> bugreport: https://bugs.openjdk.java.net/browse/JDK-8191521
> webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8191521/webrev.0/
>
> Best regards,
> Ralf
>