RFR: 8350869: os::stat doesn't follow symlinks on Windows [v3]

[David Holmes]

On Thu, 15 May 2025 13:38:20 GMT, Anton Artemov <duke at openjdk.org> wrote:

>> This PR addresses an issue on Windows, where a class file could not be loaded by a symbolic link. 
>> 
>> I added a check if a wide path is a symbolic link (soft one) and a method for dereferencing. Both os::stat() and os::open() on Windows now can handle such links.
>> 
>> Tested in tiers 1-3. A separate test added.
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8350869: Addressed reviewer's comments

Thanks for the updates, a number of style issues remaining, and some more error checking queries/concerns.

src/hotspot/os/windows/os_windows.cpp line 4616:

> 4614: static bool is_symbolic_link(const wchar_t* wide_path) {
> 4615:   WIN32_FIND_DATAW fd;
> 4616:   HANDLE f = ::FindFirstFileW(wide_path, &fd);

Do you need to check for errors here?

src/hotspot/os/windows/os_windows.cpp line 4625:

> 4623: static WCHAR* get_path_to_target(const wchar_t* wide_path) {
> 4624:   HANDLE hFile = CreateFileW(wide_path, GENERIC_READ, FILE_SHARE_READ, nullptr,
> 4625:     OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);

Suggestion:

  HANDLE hFile = CreateFileW(wide_path, GENERIC_READ, FILE_SHARE_READ, nullptr,
                             OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);

src/hotspot/os/windows/os_windows.cpp line 4625:

> 4623: static WCHAR* get_path_to_target(const wchar_t* wide_path) {
> 4624:   HANDLE hFile = CreateFileW(wide_path, GENERIC_READ, FILE_SHARE_READ, nullptr,
> 4625:     OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);

Suggestion:

  HANDLE hFile = CreateFileW(wide_path, GENERIC_READ, FILE_SHARE_READ, nullptr,
                             OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);

Parameters should line up

src/hotspot/os/windows/os_windows.cpp line 4628:

> 4626: 
> 4627:   const size_t target_path_size = ::GetFinalPathNameByHandleW(hFile, nullptr, 0,
> 4628:     FILE_NAME_NORMALIZED);

Suggestion:

  const size_t target_path_size = ::GetFinalPathNameByHandleW(hFile, nullptr, 0,  FILE_NAME_NORMALIZED);

Or else align parameters

src/hotspot/os/windows/os_windows.cpp line 4637:

> 4635:   WCHAR* path_to_target = NEW_C_HEAP_ARRAY(WCHAR, target_path_size + 1, mtInternal);
> 4636: 
> 4637:   ::GetFinalPathNameByHandleW(hFile, path_to_target, static_cast<DWORD>(target_path_size + 1),

Maybe save the return value and assert it is the expected one.

src/hotspot/os/windows/os_windows.cpp line 4721:

> 4719: 
> 4720:   if (is_symlink)
> 4721:   {

Suggestion:

  if (is_symlink) {

src/hotspot/os/windows/os_windows.cpp line 4724:

> 4722:     path_to_target = get_path_to_target(wide_path);
> 4723:     if (path_to_target == nullptr)
> 4724:     {

Suggestion:

    if (path_to_target == nullptr) {

src/hotspot/os/windows/os_windows.cpp line 4733:

> 4731: 
> 4732:   WIN32_FILE_ATTRIBUTE_DATA file_data;;
> 4733:   BOOL bret = ::GetFileAttributesExW(is_symlink && path_to_target != nullptr ? path_to_target : wide_path, GetFileExInfoStandard, &file_data);

Didn't we already return if `path_to_target` is null?

src/hotspot/os/windows/os_windows.cpp line 4738:

> 4736: 
> 4737:   if (!bret) {
> 4738:     errno = ::GetLastError();

Pre-existing but `GetLastError` could have been reset by the time we get here depending on what is inside `os::free`.

src/hotspot/os/windows/os_windows.cpp line 4931:

> 4929: 
> 4930:   if (is_symlink)
> 4931:   {

Suggestion:

  if (is_symlink) {

src/hotspot/os/windows/os_windows.cpp line 4934:

> 4932:     path_to_target = get_path_to_target(wide_path);
> 4933:     if (path_to_target == nullptr)
> 4934:     {

Suggestion:

    if (path_to_target == nullptr) {

src/hotspot/os/windows/os_windows.cpp line 4947:

> 4945: 
> 4946:   if (fd == -1) {
> 4947:     errno = ::GetLastError();

Again `GetLastError` may have been reset

test/hotspot/jtreg/runtime/LoadClass/TestSymlinkLoad.java line 67:

> 65:         OutputAnalyzer output = new OutputAnalyzer(pb.start());
> 66:         output.shouldContain("Hello World")
> 67:             .shouldHaveExitValue(0);

Style nit: The dots should align

test/hotspot/jtreg/runtime/LoadClass/TestSymlinkLoad.java line 67:

> 65:         OutputAnalyzer output = new OutputAnalyzer(pb.start());
> 66:         output.shouldContain("Hello World")
> 67:             .shouldHaveExitValue(0);

Please align dots on chained invocations. Thanks

test/hotspot/jtreg/runtime/LoadClass/TestSymlinkLoad.java line 80:

> 78:         output = new OutputAnalyzer(pb.start());
> 79:         output.shouldContain("Hello World")
> 80:             .shouldHaveExitValue(0);

Style nit: The dots should align

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25233#pullrequestreview-2845575955
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092380983
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092376411
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092381874
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092382518
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092386369
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092386979
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092387226
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092388895
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092392278
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092392674
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092392964
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092393514
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092378056
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092394571
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092378608