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

[David Holmes]

On Wed, 14 May 2025 14:54:48 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.

I've taken a first pass through this - disclaimer I don't know these Windows API's nor in general how to deal with symlinks. I have a lot of style issues that need correcting, but also an issue with error handling.

Thanks.

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

> 4612: 
> 4613: // This method checks if a wide path is actually a symbolic link
> 4614: static bool is_symbolic_link(const wchar_t* wide_path)

This is a `bool` function but you also set `errno` upon errors - how will anyone know that an error occurred?

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

> 4613: // This method checks if a wide path is actually a symbolic link
> 4614: static bool is_symbolic_link(const wchar_t* wide_path)
> 4615: {

Suggestion:

static bool is_symbolic_link(const wchar_t* wide_path) {

Hotspot-style says the { goes on the same line as the function declaration.

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

> 4617:   HANDLE f = ::FindFirstFileW(wide_path, &fd);
> 4618:   const bool result = fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT && fd.dwReserved0 == IO_REPARSE_TAG_SYMLINK;
> 4619:   if (0 == ::FindClose(f)) {

```style-nit suggestion
  if (::FindClose(f) == 0) {

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

> 4618:   const bool result = fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT && fd.dwReserved0 == IO_REPARSE_TAG_SYMLINK;
> 4619:   if (0 == ::FindClose(f)) {
> 4620:     errno = ::GetLastError();

I had not noticed this kind of error handling before. It is not obvious that `errno` is able to accept all the values `GetLastError` may report - or perhaps more accurately that anyone checking `errno` would expect to see them.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/errno-constants?view=msvc-170

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

> 4626: // This method dereferences a symbolic link
> 4627: static WCHAR* get_path_to_target(const wchar_t* wide_path)
> 4628: {

Suggestion:

static WCHAR* get_path_to_target(const wchar_t* wide_path) {

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

> 4630: 
> 4631:   hFile = CreateFileW(wide_path, GENERIC_READ, FILE_SHARE_READ, nullptr,
> 4632:     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 4638:

> 4636: 
> 4637:   if (target_path_size == 0)
> 4638:   {

Suggestion:

  if (target_path_size == 0) {

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

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

Suggestion:

        output.shouldContain("Hello World")
              .shouldHaveExitValue(0);

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

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

Suggestion:

        output.shouldContain("Hello World")
              .shouldHaveExitValue(0);

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

> 87:     public static void createLinkInSeparateFolder( final String pathToFolderForSymlink, final Path target, final String className) throws IOException {
> 88:         File theDir = new File(pathToFolderForSymlink);
> 89:         if (!theDir.exists()){

Suggestion:

        if (!theDir.exists()) {

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25233#pullrequestreview-2842618311
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090632317
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090522324
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090510941
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090628885
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090523005
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090518969
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090520141
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090527145
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090536089
PR Review Comment: https://git.openjdk.org/jdk/pull/25233#discussion_r2090536587