RFR(S): 8188122: Path length limits on Windows leads to obscure class loading failures
Calvin Cheung
calvin.cheung at oracle.com
Fri Oct 27 00:03:02 UTC 2017
Hi Thomas,
On 10/25/17, 11:54 PM, Thomas Stüfe wrote:
> Hi Calvin,
>
> this is a worthwhile addition, thank you for your work!
Thanks for your review.
>
> =========================
>
> src/hotspot/os/windows/os_windows.cpp
>
> Could you please use wchar_t instead of WCHAR?
Fixed.
>
> ---------------
> 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.
I've made the change.
>
> --------------------
>
> 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.
os::native_path() converts a path like C:\\\\somedir to C:\\somedir
So I'll need the converted path in the wchar_t case too. The freeing of
the pathbuf is being done at the end of the function or in the middle of
the wchar_t case if there's an error.
>
> -----------------------
>
> 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"\\?\"
> }
I also include the case for adding L"\\\\?\\UNC\0" at the beginning to
be consistent with libjava/canonicalize_md.c.
>
> We also need error checking to mbstowcs_s.
I've added assert like the following after the call:
assert(converted_chars == path_len, "sanity");
>
> Just for cleanliness, I would then wrap deallocation into an own function.
>
> static viud destroy_unc_path(whchar_t* s) { os::free(s); }
I've added the destroy function.
>
> 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.
I'm leaving them in os_windows.cpp since they're being used only within
that file.
>
> -----------------------
>
> FindFirstFileW:
>
> I am pretty sure that you can achieve the same result with
> GetFileAttributesExW(). It also returns WIN32_FIND_DATAW.
It actually returns WIN32_FILE_ATTRIBUTE_DATA and is very similar to
WIN32_FIND_DATAW.
>
> It is way more straightforward to use than FindFirstFileW, as it does
> not require you to write a callback hook.
I've switched to using GetFileAttributesExW().
>
> -------
>
> eval_find_data(): This is more of a generic helper function, could you
> rename this to something clearer, e.g. make_double_word() ?
Ok. I've renamed it.
>
> Also, setup_stat(), could this be renamed more clearly to something
> like WIN32_FIND_DATA_to_stat? or lowercase if this bothers you :)
I'm naming the function as file_attribute_data_to_stat() to match with
the data structure name.
>
> ==================================
> 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.
I've added an assert statement.
>
> ===================
Here's an updated webrev:
http://cr.openjdk.java.net/~ccheung/8188122/webrev.02/
thanks,
Calvin
>
> Thanks!
>
> Thomas
>
>
>
>
>
>
> On Wed, Oct 25, 2017 at 9:48 PM, Calvin Cheung
> <calvin.cheung at oracle.com <mailto: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/
> <http://cr.openjdk.java.net/%7Eccheung/8188122/webrev.01/>
>
> It passed hs-tier2 testing using mach5.
>
> thanks,
> Calvin
>
> [1]
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#MAX_PATH
> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx#MAX_PATH>
>
> [2]
> https://social.msdn.microsoft.com/Forums/vstudio/en-US/3c093ea9-f0aa-446d-b648-2dabe8480430/stat-and-long-names?forum=vcgeneral
> <https://social.msdn.microsoft.com/Forums/vstudio/en-US/3c093ea9-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
> <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\\LoadClass\\LongPath.d\\xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 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\\LoadClass\\LongPath.d\\xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\\xxxxx
>
> webrev:
> http://cr.openjdk.java.net/~ccheung/8188122/webrev.00/
> <http://cr.openjdk.java.net/%7Eccheung/8188122/webrev.00/>
>
> Testing:
> JPRT (including the new test)
>
> thanks,
> Calvin
>
>
More information about the hotspot-runtime-dev
mailing list