Improve error handling in OpenProcess method : Windows32Exception - The parameter is incorrect #1700

[marktech0813]

I found the Windows code path that throws in getProcessFilePath(HWND) and implemented a defensive fix so your logs won’t fill with Win32Exceptions anymore. Now, when OpenProcess fails with ERROR_INVALID_PARAMETER (87), it is treated like ACCESS_DENIED: we either retry with PROCESS_QUERY_LIMITED_INFORMATION, or return an empty path if access still fails. This covers the race where a PID disappears between enumeration and open, or other transient/unsupported cases.

In contrib/platform/src/com/sun/jna/platform/WindowUtils.java, updated W32WindowUtils.getProcessFilePath:
On first OpenProcess failure, if GetLastError() is ACCESS_DENIED or INVALID_PARAMETER, we attempt PROCESS_QUERY_LIMITED_INFORMATION.
If the second OpenProcess also fails with ACCESS_DENIED or INVALID_PARAMETER, we return an empty string instead of throwing.
Only other error codes still throw a Win32Exception.

[JaredDavis22]

Hi,
I believe CHANGES.MD needs an update as well.

Is your email valid? From: helloJetBase-tech 178346048+marktech0813@users.noreply.github.com That seems to be a frequent comment from @matthiasblaesing.

What about using a switch instead of adding a local variable?

[marktech0813]

hi,

[marktech0813]

I replaced the local variable with a switch on GetLastError in getProcessFilePath(HWND) so we branch on ACCESS_DENIED and INVALID_PARAMETER without storing the error.
I added a bug-fix entry under “Next Release (5.19.0) → Bug Fixes” in CHANGES.md referencing #1700.

[marktech0813]

Let me know if you’d prefer we still cache the error once per branch to be extra defensive, but functionally it’s now switch-based and CHANGES is updated.

[BugsBeGone]

While we're finessing, how about an early break and fall-through to reduce duplicate code? Within the first switch, rather than checking for null, just break out early if process is assigned. Then the nested switch can just check for the 2 cases, else fall through to the outer switch's default. Maybe better if I just show the code:

switch (Kernel32.INSTANCE.GetLastError()) {
    case WinNT.ERROR_ACCESS_DENIED:
    case WinError.ERROR_INVALID_PARAMETER:
        process = Kernel32.INSTANCE.OpenProcess(
                WinNT.PROCESS_QUERY_LIMITED_INFORMATION,
                false,
                pid.getValue());
        if (process != null) {
            break;
        }
        switch (Kernel32.INSTANCE.GetLastError()) {
            case WinNT.ERROR_ACCESS_DENIED:
            case WinError.ERROR_INVALID_PARAMETER:
                return "";
        }
        /* if above didn't already break or return, fall through to default */
    default:
        throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}

[marktech0813]

Yes. Functionally identical, but cleaner:

• Less duplication and shallower nesting via early break and fall-through.
• No extraneous local state; logic is driven directly by GetLastError.
• Readability improved while preserving exact semantics (retry on ACCESS_DENIED/INVALID_PARAMETER, otherwise throw; return "" on second failure).

[matthiasblaesing]

The problem is real and this stack overflow post gives some possible reasons: https://stackoverflow.com/questions/4988082/openprocess-error-87-invalid-parameter

The use the correct parameter to fetch the PID, but the other to reasons (window from system process or different user/terminal services) sounds reasonable.

So I agree with the general idea here.

Nitpicks:

• see inline comments
• please ensure that the commits have valid author information. Valid means a realname and email (this was already brought up by @JaredDavis22 but ignored). You can see the "real" author information in the patch view for a PR. In this case: https://patch-diff.githubusercontent.com/raw/java-native-access/jna/pull/1701.patch

Please squash the commits into a single commit.

[marktech0813]

okay.

[matthiasblaesing]

Please squash and update the author information.