RFR(S): 8188122: Path length limits on Windows leads to obscure class loading failures

[Thomas Stüfe]

Hi Calvin,

this is a worthwhile addition, thank you for your work!

=========================

src/hotspot/os/windows/os_windows.cpp

Could you please use wchar_t instead of WCHAR?

---------------
in os::stat():

This cumbersome malloc/strcpy sequence:

!   size_t path_len = strlen(path) + 1;
+   char* pathbuf = (char*)os::malloc(path_len * sizeof(char), mtInternal);
+   if (pathbuf == NULL) {
+     errno = ENOMEM;
      return -1;
    }
    os::native_path(strcpy(pathbuf, path));

 can be reduced to a simple strdup:

char* pathbuf = os::strdup(path, mtInternal);
if (pathbuf == NULL) {
  errno = ENOMEM;
  return -1;
}
os::native_path(pathbuf);

This also would separate strcpy() from os::native_path() which is a bit
unreadable.

--------------------

The single-byte path (strdup, ::stat()), together with its free(), can be
moved inside the (strlen(path) < MAX_PATH) condition. os::native_path will
not change the path length (it better not, as it operates on the input
buffer). That avoids  having two allocations on the doublebyte path.

-----------------------

But seeing that translation to UNC paths is something we might want more
often, I would combine allocation and UNC prefix adding to one function
like this, to avoid the memove and increase reusability:

// Creates an unc path from a single byte path. Return buffer is allocated
in C heap and needs to be freed by caller. Returns NULL on error.
static whchar_t* create_unc_path(const char* s) {
  - does s start with \\?\ ?
- yes:
            - os::malloc(strlen(s) + 1) and mbstowcs_s
        - no:
            - os::malloc(strlen(s) + 1 + 4), mbstowcs_s to fourth position
in string, prefix with L"\\?\"
}

We also need error checking to mbstowcs_s.

Just for cleanliness, I would then wrap deallocation into an own function.

static viud destroy_unc_path(whchar_t* s) { os::free(s); }

These two functions could be candidates of putting into os::win32
namespace, as this form of ANSI->UNC path translation is quite common -
whoever wants to use the xxxxW() functions must do this.

-----------------------

FindFirstFileW:

I am pretty sure that you can achieve the same result with
GetFileAttributesExW(). It also returns WIN32_FIND_DATAW.

It is way more straightforward to use than FindFirstFileW, as it does not
require you to write a callback hook.

-------

eval_find_data(): This is more of a generic helper function, could you
rename this to something clearer, e.g. make_double_word() ?

Also, setup_stat(), could this be renamed more clearly to something like
WIN32_FIND_DATA_to_stat? or lowercase if this bothers you :)

==================================
src/hotspot/share/classfile/classLoader.cpp

In ClassPathDirEntry::open_stream(), I would feel better if we were
asserting _dir and name to be != NULL before feeding it to strlen.

===================

Thanks!

Thomas






On Wed, Oct 25, 2017 at 9:48 PM, Calvin Cheung <calvin.cheung at oracle.com>
wrote:

> I've reworked this fix by using the Unicode version of open (wopen) to
> handle path name longer than max path with the path prefixed to indicate an
> extended length path as described in [1].
>
> The Unicode version of stat (wstat) doesn't work well with long path [2].
> So FindFirstFileW will be used.The data in WIN32_FIND_DATA returned from
> FindFirstFileW needs to be transferred to the stat structure since the
> caller expects a return stat structure and other platforms return a stat
> structure.
>
> In classLoader.cpp, calculate the size of buffer required instead of
> limiting it to JVM_MAXPATHLEN.
> In os_windows.cpp, dynamically allocate buffers in os::open and os::stat.
>
> updated webrev: http://cr.openjdk.java.net/~ccheung/8188122/webrev.01/
>
> It passed hs-tier2 testing using mach5.
>
> thanks,
> Calvin
>
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa3
> 65247(v=vs.85).aspx#MAX_PATH
> [2] https://social.msdn.microsoft.com/Forums/vstudio/en-US/3c093
> ea9-f0aa-446d-b648-2dabe8480430/stat-and-long-names?forum=vcgeneral
>
>
> On 10/16/17, 3:15 PM, Calvin Cheung wrote:
>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8188122
>>
>> Adding a warning message if the full path or the directory length exceeds
>> MAX_PATH on windows.
>>
>> Example warning messages.
>>
>> 1) The full path exceeds MAX_PATH:
>>
>> Java HotSpot(TM) 64-Bit Server VM warning: construct full path name
>> failed: total length 270 exceeds max length of 260
>>     dir T:\\testoutput\\jtreg\\JTwork\\classes\\2\\runtime\\LoadClas
>> s\\LongPath.d\\xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> length 259
>>     name Hello.class length 11
>>
>> 2) The directory path exceeds MAX_PATH:
>>
>> Java HotSpot(TM) 64-Bit Server VM warning: os::stat failed: path length
>> 265 exceeds max length 260
>>     path T:\\testoutput\\jtreg\\JTwork\\classes\\2\\runtime\\LoadClas
>> s\\LongPath.d\\xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> xxxxxxxxxxxxxxxxxxxxxxxxxxxx\\xxxxx
>>
>> webrev:
>>     http://cr.openjdk.java.net/~ccheung/8188122/webrev.00/
>>
>> Testing:
>>     JPRT (including the new test)
>>
>> thanks,
>> Calvin
>>
>