RFR: 8364816: GetLastError() in os_windows.cpp should not store value to errno [v4]

[David Holmes]

On Wed, 27 Aug 2025 13:09:39 GMT, Anton Artemov <duke at openjdk.org> wrote:

>> Hi, please consider the following changes:
>> 
>> In `os_windows.cpp` in a few places results returned by `GetLastError()` are stored to `errno`. However, `errno` has no relation to `GetLastError()`, and their ranges are not the same.
>> 
>> Results of `GetLastError()` should be stored into variables of type `DWORD`.
>> 
>> The removed section in `src/hotspot/share/cds/aotClassLocation.cpp` was relying on values returned by `GetLastError()` and stored to `errno` in `os::stat()`. Though the logic was correct, these values should not be stored to `errno`. The functionality is preserved by storing a **valid** value `ENOENT` to `errno` in `os::stat()`. 
>> 
>> Tested in tiers 1 - 3.
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8364816: Modified case when ENOENT value is stored to errno.

Things are never simple. :)

The basic change of not setting `errno` from `GetLastError` is good. That is a bug that had to be fixed.

I think your approach for setting `errno` when callers expect it to be set is also good (but see below). Most callers of `os::stat` care only about success or failure - not the details of why (which you have recorded in the logging anyway if the user/developer is interested).

You also fixed the `aotClassLocation.cpp` hack because you should never have to check `errno` for Windows specific error codes - so that is an extra bonus IMO.

The more general problem of how to map `GetLastError` to `errno` when callers expect to check `errno`, may still remain in other places. That's a separate issue IMO. But I also note that in JDK 8 code we do perform such a mapping where needed (we have more cases to deal with post JDK 8 but it is worth looking at the old code).

However, this has highlighted a giant hole in the way `errno` is being used: we have cleanup code that will likely overwrite `errno` before the caller could get to check it! And if cleanup code doesn't then logging code will. This likely affects non-Windows code too - though it may be `ErrnoPreserver` is being used in enough places there. This seems to be a fairly new problem - I don't see it in JDK 21. I have filed [JDK-8366272](https://bugs.openjdk.org/browse/JDK-8366272)

I have flagged one thing, in relation to `_wopen` that needs fixing, but otherwise I think this PR is okay in this form. But then we need the follow up to properly manage `errno`.

Thanks

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

> 5033:   if (fd == -1) {
> 5034:     DWORD errcode = ::GetLastError();
> 5035:     log_debug(os)("os::open() failed to _wopen: GetLastError->%lu.", errcode);

This is not actually needed. `_wopen` sets `errno` not `GetlastError`, so the original code is just wrong.

However, (and this is a general problem with errno and cleanup actions) if we log the error, we will modify errno so even if a caller is checking errno they won't get what they expect. Even if not logging the `os::free` can also potentially modify errno so this use of errno is inherently flawed!

A proper fix (and likely applies to non-Windows code too) would be to save errno and restore only upon returning.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/26901#pullrequestreview-3162817653
PR Review Comment: https://git.openjdk.org/jdk/pull/26901#discussion_r2305958909