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

[Anton Artemov]

On Fri, 16 May 2025 06:02:08 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8350869: Addressed reviewer's comments
>
> 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?

Thanks for spotting this, I added error checking for every call to Windows API function.

> 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

Addressed in the latest commit.

> 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);

Addressed in the latest commit.

> 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

Addressed in the latest commit.

> 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.

We do not have an expected value here.

> src/hotspot/os/windows/os_windows.cpp line 4721:
> 
>> 4719: 
>> 4720:   if (is_symlink)
>> 4721:   {
> 
> Suggestion:
> 
>   if (is_symlink) {

Addressed in the latest commit.

> 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) {

Addressed in the latest commit.

> 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?

Correct, we only need to check if is_symlink is true or not.

> 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`.

Yes, apparently one has to call `GetLastError` directly after calling the method whose errors we want to see. I addressed it in the latest commit.

> src/hotspot/os/windows/os_windows.cpp line 4931:
> 
>> 4929: 
>> 4930:   if (is_symlink)
>> 4931:   {
> 
> Suggestion:
> 
>   if (is_symlink) {

Addressed in the latest commit.

> 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) {

Addressed in the latest commit.

> src/hotspot/os/windows/os_windows.cpp line 4947:
> 
>> 4945: 
>> 4946:   if (fd == -1) {
>> 4947:     errno = ::GetLastError();
> 
> Again `GetLastError` may have been reset

Yes, `GetLastError` should be called directly after the method whose errors we want to see. Addressed.

> 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

Addressed in the latest commit.

> 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

Addressed in the latest commit.

> 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

Addressed in the latest commit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092775468
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092775362
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092775782
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092775206
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092775057
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774940
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774827
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774701
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774562
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774436
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774302
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774172
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092774006
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092775677
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2092775566