Kernel#GetLastError() always returns ERROR_SUCCESS - Native#getLastError() returns the correct code

[lgoldstein]

Advapi32Util has 2 methods that wrap the above mentioned API calls - getAccountBySid/Name respectively. The 2 methods work more or less the same - they make an initial call to the LookupAccountXXX method in order to query how much memory they need to allocate. The code expects the initial call to fail with an error code of ERROR_INSUFFICIENT_BUFFER (122). The code checks this by invoking Kernel32.INSTANCE.GetLastError().

Here is the strange bit: the call to LookupAccountXXX does fail, but the reported error code from the Kernel32.INSTANCE.GetLastError() invocation is zero - i.e., success. If I replace the call to Kernel32.INSTANCE.GetLastError() with Native.getLastError(), then indeed ERROR_INSUFFICIENT_BUFFER is reported and the code continues successfully.

This behavior is reproduced every time on my computer - Windows 10 Home edition 64 bit with VS 2015 update 1 - whenever I run Advapi32UtilTest#testGetAccountBySid/Name (respectively).

[lgoldstein]

Just found out a few more locations where this happens:

• Advapi32Util#getSecurityDescriptorForFile - the initial call to Advapi32.INSTANCE.GetFileSecurity is also expected to fail with ERROR_INSUFFICIENT_BUFFER
• Advapi32Util#getUserName - if one provides a small buffer to the initial call to Advapi32.INSTANCE.GetUserNameW the same kind of failure is expected.

In both these cases (and I am pretty sure all the other) replacing the call to Kernel32.INSTANCE.GetLastError() with Native.getLastError() seems to fix the problem. This makes me wonder whether some bug was introduced in the Kernel32.INSTANCE.GetLastError() in general or just when it comes to ERROR_INSUFFICIENT_BUFFER. I believe this issue warrants further investigation...

[twall]

JNA intercepts calls to GetLastError and replaces them with Native.getLastError(). Perhaps that is now not taking place?

Sent from my iPhone

On Mar 2, 2016, at 4:21 PM, Lyor Goldstein notifications@github.com wrote:

Advapi32Util has 2 methods that wrap the above mentioned API calls - getAccountBySid/Name respectively. The 2 methods work more or less the same - they make an initial call to the LookupAccountXXX method in order to query how much memory they need to allocate. The code expects the initial call to fail with an error code of ERROR_INSUFFICIENT_BUFFER (122). The code checks this by invoking Kernel32.INSTANCE.GetLastError().

Here is the strange bit: the call to LookupAccountXXX does fail, but the reported error code from the Kernel32.INSTANCE.GetLastError() invocation is zero - i.e., success. If I replace the call to Kernel32.INSTANCE.GetLastError() with Native.getLastError(), then indeed ERROR_INSUFFICIENT_BUFFER is reported and the code continues successfully.

This behavior is reproduced every time on my computer - Windows 10 Home edition 64 bit with VS 2015 update 1 - whenever I run Advapi32UtilTest#testGetAccountBySid/Name (respectively).

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

Not my area of expertise... - definitely worth looking into it. I am baffled that nobody reported this - is there no continuous integration for Windows APIs... ?

[twall]

CI doesn’t currently have windows coverage. It’s mostly DB or myself running checks manually.

On Mar 3, 2016, at 12:10 AM, Lyor Goldstein notifications@github.com wrote:

Not my area of expertise... - definitely worth looking into it. I am baffled that nobody reported this - is there no continuous integration for Windows APIs... ?

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

Then it seems there is such a problem - at least on my setup, all the tests that rely on a specific error code to be returned fail with the exact same value (zero) when they call Kernel#GetLastError(). Just out of curiosity, I have modified some of them to use Native#getLastError() and it fixed the problem.

[twall]

I’ll try on my system and see if I get the same failures. Which test class(es)?

On Mar 3, 2016, at 7:15 AM, Lyor Goldstein notifications@github.com wrote:

Then it seems there is such a problem - at least on my setup, all the tests that rely on a specific error code to be returned fail with the exact same value (zero) when they call Kernel#GetLastError(). Just out of curiosity, I have modified some of them to use Native#getLastError() and it fixed the problem.

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

Here are a few examples:

• Advapi32Test - testCloseServiceHandle, testAccessCheck, testOpenSCManager
  If you replace the call to Kernel.INSTANCE.GetLastError() with Native.getLastError() the tests succeed
• Advapi32UtilTest - testExecuteAccess, testReadAccess, etc..
  If you go to Advapi32Util and replace all calls to Kernel.INSTANCE.GetLastError() with Native.getLastError() the tests succeed

BTW, since I cannot compile the native code, I am using the jnidispatch.dll that I extracted from the checked in lib/native/win32-x86-64.jar and using -Djna.boot.library.path="path/to/folder/containing/jnidispatch"

[lgoldstein]

Just out of curiosity, I modified the code for Library.Handler#invoke as follows:

// Intercept Object methods
...
} else if ("GetLastError".equals(method.getName())
          && ((inArgs == null) || (inArgs.length == 0))) {
           return Integer.valueOf(Native.getLastError());
}

and it fixed all the tests.

[twall]

The original workaround is in NativeLibrary, which does a similar thing around line 108:

https://github.com/java-native-access/jna/blob/master/src/com/sun/jna/NativeLibrary.java#L108

On Mar 3, 2016, at 7:52 AM, Lyor Goldstein notifications@github.com wrote:

Just out of curiosity, I modified the code for Library.Handler#invoke as follows:

// Intercept Object methods
...

}
else if ("GetLastError".equals(method.
getName())

&& ((inArgs == null) || (inArgs.length == 0
))) {

return Integer.valueOf(Native.
getLastError());
}

and it fixed all the tests.

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

This is actually the error line - the wrong invoke method is overridden

The original line is

Function f = new Function(this, "GetLastError", Function.ALT_CONVENTION, encoding) {
                        @Override
                        Object invoke(Object[] args, Class<?> returnType, boolean b) {
                            return Integer.valueOf(Native.getLastError());
                        }
                    };

Whereas the correct code should be

Function f = new Function(this, "GetLastError", Function.ALT_CONVENTION, encoding) {
                        @Override
                        Object invoke(Method invokingMethod, Class<?>[] paramTypes, Class<?> returnType, Object[] inArgs, Map<String, ?> options) {
                            return Integer.valueOf(Native.getLastError());
                        }
                    };

Although to be on the safe side we should probably override both methods. Would you like me to issue a bugfix pull request or do you want to do it.

[lgoldstein]

BTW, here is the commit that caused the problem: 50eb820

[twall]

Apparently something not caught in some round of refactoring, and likely not covered by a unit test.

Please do fix it.

On Mar 3, 2016, at 8:44 AM, Lyor Goldstein notifications@github.com wrote:

This is actually the error line - the wrong invoke method is overridden

The original line is

Function f = new Function(this, "GetLastError", Function.ALT_CONVENTION
, encoding) {

@OverRide

Object invoke(Object[] args, Class<?> returnType, boolean b
) {

return Integer.valueOf(Native.
getLastError());
}
};

Whereas the correct code should be

Function f = new Function(this, "GetLastError", Function.ALT_CONVENTION
, encoding) {

@OverRide

Object invoke(Method invokingMethod, Class[] paramTypes, Class returnType, Object[] inArgs, Map<String, ?> options
) {

return Integer.valueOf(Native.
getLastError());
}
};

Although to be on the safe side we should probably override both methods. Would you like me to issue a bugfix pull request or do you want to do it.

—
Reply to this email directly or view it on GitHub.

[twall]

Yup, that would be me. Good catch.

On Mar 3, 2016, at 8:54 AM, Lyor Goldstein notifications@github.com wrote:

BTW, here is the commit that caused the problem: 50eb820

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

Should I create a pull request or just fix and push (do I have push rights to the master...?)

[twall]

I think there’s still a gap if you direct-map “kernel32”, but it’d likely have to be patched in native code.

On Mar 3, 2016, at 8:44 AM, Lyor Goldstein notifications@github.com wrote:

This is actually the error line - the wrong invoke method is overridden

The original line is

Function f = new Function(this, "GetLastError", Function.ALT_CONVENTION
, encoding) {

@OverRide

Object invoke(Object[] args, Class<?> returnType, boolean b
) {

return Integer.valueOf(Native.
getLastError());
}
};

Whereas the correct code should be

Function f = new Function(this, "GetLastError", Function.ALT_CONVENTION
, encoding) {

@OverRide

Object invoke(Method invokingMethod, Class[] paramTypes, Class returnType, Object[] inArgs, Map<String, ?> options
) {

return Integer.valueOf(Native.
getLastError());
}
};

Although to be on the safe side we should probably override both methods. Would you like me to issue a bugfix pull request or do you want to do it.

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

Waiting on PR-606 and then will apply the fix directly to the master (unless you want me to do it via a separate PR)

[twall]

You have rights on the main repo, so you don’t have to work from a fork, but I still prefer you do a branch->PR->merge.

On Mar 3, 2016, at 9:36 AM, Lyor Goldstein notifications@github.com wrote:

Waiting on PR-606 and then will apply the fix directly to the master (unless you want me to do it via a separate PR)

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

Will do so.