Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added standard Kernel32Util#closeHandle method that throws an exception if failed to close the handle #614

Merged
merged 1 commit into from
Mar 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ Features
* [#595](https://github.com/java-native-access/jna/pull/595): Allow calling COM methods/getters requiring hybrid calling (METHOD+PROPERTYGET) - [@matthiasblaesing](https://github.com/matthiasblaesing).
* [#582](https://github.com/java-native-access/jna/pull/582): Mavenize the build process - Phase 1: building the native code via Maven [@lgoldstein](https://github.com/lgoldstein).
* [#606](https://github.com/java-native-access/jna/pull/606): Added Kerne32Util method to facilitate checking that calls to LocalFree/GlobalFree are successful [@lgoldstein](https://github.com/lgoldstein).
* [#612](https://github.com/java-native-access/jna/pull/612): Kernel32Util#freeLocal/GlobalMemory always throws Win32Exception if failed [@lgoldstein](https://github.com/lgoldstein).
* [#612](https://github.com/java-native-access/jna/pull/612): 'Kernel32Util#freeLocal/GlobalMemory' always throws Win32Exception if failed [@lgoldstein](https://github.com/lgoldstein).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those backticks are the wrong kind ;) Also above there's a typo, Kerne32Util is missing an L :)

* [#608](https://github.com/java-native-access/jna/pull/608): Mavenize the build process - change parent and native pom artifactId/name to differentiate in IDE and build tools. [@bhamail](https://github.com/bhamail)
* [#613](https://github.com/java-native-access/jna/pull/613): Make Win32Exception extend LastErrorException [@lgoldstein](https://github.com/lgoldstein).
* [#613](https://github.com/java-native-access/jna/pull/614): Added standard 'Kernel32Util#closeHandle' method that throws a Win32Exception if failed to close the handle [@lgoldstein](https://github.com/lgoldstein).

Bug Fixes
---------
Expand Down
87 changes: 54 additions & 33 deletions contrib/platform/src/com/sun/jna/platform/win32/Advapi32Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -478,28 +478,45 @@ public static Account getTokenAccount(HANDLE hToken) {
*/
public static Account[] getCurrentUserGroups() {
HANDLEByReference phToken = new HANDLEByReference();
Win32Exception err = null;
try {
// open thread or process token
HANDLE threadHandle = Kernel32.INSTANCE.GetCurrentThread();
if (!Advapi32.INSTANCE.OpenThreadToken(threadHandle,
TOKEN_DUPLICATE | TOKEN_QUERY, true, phToken)) {
if (W32Errors.ERROR_NO_TOKEN != Kernel32.INSTANCE
.GetLastError()) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
int rc = Kernel32.INSTANCE.GetLastError();
if (rc != W32Errors.ERROR_NO_TOKEN) {
throw new Win32Exception(rc);
}

HANDLE processHandle = Kernel32.INSTANCE.GetCurrentProcess();
if (!Advapi32.INSTANCE.OpenProcessToken(processHandle,
TOKEN_DUPLICATE | TOKEN_QUERY, phToken)) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}
}

return getTokenGroups(phToken.getValue());
} catch(Win32Exception e) {
err = e;
throw err; // re-throw in order to invoke finally block
} finally {
if (phToken.getValue() != WinBase.INVALID_HANDLE_VALUE) {
if (!Kernel32.INSTANCE.CloseHandle(phToken.getValue())) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
HANDLE hToken = phToken.getValue();
if (!WinBase.INVALID_HANDLE_VALUE.equals(hToken)) {
try {
Kernel32Util.closeHandle(hToken);
} catch(Win32Exception e) {
if (err == null) {
err = e;
} else {
err.addSuppressed(e);
}
}
}

if (err != null) {
throw err;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure nobody loves this whole way we handle closing and re-throwing, maybe there's a pattern we can implement part of a future PR where this gets wrapped into some boilerplate implementation ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were using Java 8 I could have done it using a functional interface...

}
}

Expand Down Expand Up @@ -2341,64 +2358,68 @@ else if ((securityDescriptor.Control & SE_SACL_PROTECTED) == 0) {
* @return true if has access, otherwise false
*/
public static boolean accessCheck(File file, AccessCheckPermission permissionToCheck) {
boolean hasAccess = false;
final Memory securityDescriptorMemoryPointer = getSecurityDescriptorForFile(file.getAbsolutePath().replaceAll("/", "\\"));
Memory securityDescriptorMemoryPointer = getSecurityDescriptorForFile(file.getAbsolutePath().replace('/', '\\'));

HANDLEByReference openedAccessToken = null;
final HANDLEByReference duplicatedToken = new HANDLEByReference();
HANDLEByReference openedAccessToken = new HANDLEByReference();
HANDLEByReference duplicatedToken = new HANDLEByReference();
Win32Exception err = null;
try{
openedAccessToken = new HANDLEByReference();

final int desireAccess = TOKEN_IMPERSONATE | TOKEN_QUERY | TOKEN_DUPLICATE | STANDARD_RIGHTS_READ;
if(!Advapi32.INSTANCE.OpenProcessToken(Kernel32.INSTANCE.GetCurrentProcess(), desireAccess, openedAccessToken)) {
int desireAccess = TOKEN_IMPERSONATE | TOKEN_QUERY | TOKEN_DUPLICATE | STANDARD_RIGHTS_READ;
HANDLE hProcess = Kernel32.INSTANCE.GetCurrentProcess();
if (!Advapi32.INSTANCE.OpenProcessToken(hProcess, desireAccess, openedAccessToken)) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}

if(!Advapi32.INSTANCE.DuplicateToken(openedAccessToken.getValue(), SECURITY_IMPERSONATION_LEVEL.SecurityImpersonation, duplicatedToken)) {
if (!Advapi32.INSTANCE.DuplicateToken(openedAccessToken.getValue(), SECURITY_IMPERSONATION_LEVEL.SecurityImpersonation, duplicatedToken)) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}

final GENERIC_MAPPING mapping = new GENERIC_MAPPING();
GENERIC_MAPPING mapping = new GENERIC_MAPPING();
mapping.genericRead = new DWORD(FILE_GENERIC_READ);
mapping.genericWrite = new DWORD(FILE_GENERIC_WRITE);
mapping.genericExecute = new DWORD(FILE_GENERIC_EXECUTE);
mapping.genericAll = new DWORD(FILE_ALL_ACCESS);

final DWORDByReference rights = new DWORDByReference(new DWORD(permissionToCheck.getCode()));
DWORDByReference rights = new DWORDByReference(new DWORD(permissionToCheck.getCode()));
Advapi32.INSTANCE.MapGenericMask(rights, mapping);

final PRIVILEGE_SET privileges = new PRIVILEGE_SET(1);
PRIVILEGE_SET privileges = new PRIVILEGE_SET(1);
privileges.PrivilegeCount = new DWORD(0);
final DWORDByReference privilegeLength = new DWORDByReference(new DWORD(privileges.size()));
DWORDByReference privilegeLength = new DWORDByReference(new DWORD(privileges.size()));

final DWORDByReference grantedAccess = new DWORDByReference();
final BOOLByReference result = new BOOLByReference();
if(!Advapi32.INSTANCE.AccessCheck(securityDescriptorMemoryPointer,
DWORDByReference grantedAccess = new DWORDByReference();
BOOLByReference result = new BOOLByReference();
if (!Advapi32.INSTANCE.AccessCheck(securityDescriptorMemoryPointer,
duplicatedToken.getValue(),
rights.getValue(),
mapping,
privileges, privilegeLength, grantedAccess, result)) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}

hasAccess = result.getValue().booleanValue();

return result.getValue().booleanValue();
} catch(Win32Exception e) {
err = e;
throw err; // re-throw so finally block executed
} finally {

if(openedAccessToken != null && openedAccessToken.getValue() != null) {
Kernel32.INSTANCE.CloseHandle(openedAccessToken.getValue());
try {
Kernel32Util.closeHandleRefs(openedAccessToken, duplicatedToken);
} catch(Win32Exception e) {
if (err == null) {
err = e;
} else {
err.addSuppressed(e);
}
}

if(duplicatedToken != null && duplicatedToken.getValue() != null) {
Kernel32.INSTANCE.CloseHandle(duplicatedToken.getValue());
if (securityDescriptorMemoryPointer != null) {
securityDescriptorMemoryPointer.clear();
}

if(securityDescriptorMemoryPointer != null) {
securityDescriptorMemoryPointer.clear();
if (err != null) {
throw err;
}
}

return hasAccess;
}

/**
Expand Down
5 changes: 3 additions & 2 deletions contrib/platform/src/com/sun/jna/platform/win32/Kernel32.java
Original file line number Diff line number Diff line change
Expand Up @@ -735,14 +735,15 @@ boolean DuplicateHandle(HANDLE hSourceProcessHandle, HANDLE hSourceHandle,
int dwDesiredAccess, boolean bInheritHandle, int dwOptions);

/**
* The CloseHandle function closes an open object handle.
* Closes an open object handle.
*
* @param hObject
* Handle to an open object. This parameter can be a pseudo
* handle or INVALID_HANDLE_VALUE.
* @return If the function succeeds, the return value is nonzero. If the
* function fails, the return value is zero. To get extended error
* information, call GetLastError.
* information, call {@code GetLastError}.
* @see <A HREF="https://msdn.microsoft.com/en-us/library/windows/desktop/ms724211(v=vs.85).aspx">CloseHandle</A>
*/
boolean CloseHandle(HANDLE hObject);

Expand Down
145 changes: 122 additions & 23 deletions contrib/platform/src/com/sun/jna/platform/win32/Kernel32Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,90 @@ public static void freeGlobalMemory(Pointer ptr) {
}
}

/**
* Closes all referenced handles. If an exception is thrown for
* a specific handle, then it is accumulated until all
* handles have been closed. If more than one exception occurs,
* then it is added as a suppressed exception to the first one.
* Once closed all handles, the accumulated exception (if any) is thrown
*
* @param refs The references to close
* @see #closeHandleRef(HANDLEByReference)
*/
public static void closeHandleRefs(HANDLEByReference... refs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should just be called closeHandles? After-all we can have multiple signatures if we need to with different parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but decided it would be easier to understand the code - if you feel strongly about it I have no major objection to renaming it to closeHandles

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think there’s a problem understanding it when simply named “closeHandles"; the goal is to close handles, regardless of the form used to specify the handles.

I generally don’t like codifying parameter types into function names.

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

In contrib/platform/src/com/sun/jna/platform/win32/Kernel32Util.java:

@@ -81,6 +81,90 @@ public static void freeGlobalMemory(Pointer ptr) {
}

 /**
  • \* Closes all referenced handles. If an exception is thrown for
    
  • \* a specific handle, then it is accumulated until all
    
  • \* handles have been closed. If more than one exception occurs,
    
  • \* then it is added as a suppressed exception to the first one.
    
  • \* Once closed all handles, the accumulated exception (if any) is thrown
    
  • *
    
  • \* @param refs The references to close
    
  • \* @see #closeHandleRef(HANDLEByReference)
    
  • */
    
  • public static void closeHandleRefs(HANDLEByReference... refs) {

I thought about that, but decided it would be easier to understand the code - if you feel strongly about it I have no major objection to renaming it to closeHandles


Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, @lgoldstein you should change it. Thanks!

Win32Exception err = null;
for (HANDLEByReference r : refs) {
try {
closeHandleRef(r);
} catch(Win32Exception e) {
if (err == null) {
err = e;
} else {
err.addSuppressed(e);
}
}
}

if (err != null) {
throw err;
}
}
/**
* Closes the handle in the reference
*
* @param ref The handle reference - ignored if {@code null}
* @see #closeHandle(HANDLE)
*/
public static void closeHandleRef(HANDLEByReference ref) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just closeHandle?

closeHandle((ref == null) ? null : ref.getValue());
}

/**
* Invokes {@link #closeHandle(HANDLE)} on each handle. If an exception
* is thrown for a specific handle, then it is accumulated until all
* handles have been closed. If more than one exception occurs, then it
* is added as a suppressed exception to the first one. Once closed all
* handles, the accumulated exception (if any) is thrown
*
* @param handles The handles to be closed
* @see Throwable#getSuppressed()
*/
public static void closeHandles(HANDLE... handles) {
Win32Exception err = null;
for (HANDLE h : handles) {
try {
closeHandle(h);
} catch(Win32Exception e) {
if (err == null) {
err = e;
} else {
err.addSuppressed(e);
}
}
}

if (err != null) {
throw err;
}
}

/**
* Invokes {@link Kernel32#CloseHandle(HANDLE)} and checks the success code.
* If not successful, then throws a {@link Win32Exception} with the
* {@code GetLastError} value
*
* @param h The handle to be closed - ignored if {@code null}
*/
public static void closeHandle(HANDLE h) {
if (h == null) {
return;
}

if (!Kernel32.INSTANCE.CloseHandle(h)) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}
}

/**
* Format a message from the value obtained from
* {@link Kernel32#GetLastError()} or {@link Native#getLastError()}.
Expand Down Expand Up @@ -220,6 +304,7 @@ public static int getFileType(String fileName) throws FileNotFoundException {
}

HANDLE hFile = null;
Win32Exception err = null;
try {
hFile = Kernel32.INSTANCE.CreateFile(fileName, WinNT.GENERIC_READ,
WinNT.FILE_SHARE_READ, new WinBase.SECURITY_ATTRIBUTES(),
Expand All @@ -232,25 +317,36 @@ public static int getFileType(String fileName) throws FileNotFoundException {

int type = Kernel32.INSTANCE.GetFileType(hFile);
switch (type) {
case WinNT.FILE_TYPE_UNKNOWN:
int err = Kernel32.INSTANCE.GetLastError();
switch (err) {
case WinError.NO_ERROR:
break;
default:
throw new Win32Exception(err);
}
case WinNT.FILE_TYPE_UNKNOWN:
int rc = Kernel32.INSTANCE.GetLastError();
switch (rc) {
case WinError.NO_ERROR:
break;
default:
throw new Win32Exception(rc);
}
// fall-thru

default:
return type;
}
} catch(Win32Exception e) {
err = e;
throw err; // re-throw so finally block executed
} finally {
if (hFile != null) {
if (!Kernel32.INSTANCE.CloseHandle(hFile)) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
try {
closeHandle(hFile);
} catch(Win32Exception e) {
if (err == null) {
err = e;
} else {
err.addSuppressed(e);
}
}

if (err != null) {
throw err;
}
}
}

Expand Down Expand Up @@ -942,16 +1038,17 @@ public static List<Tlhelp32.MODULEENTRY32W> getModules(int processID) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}

List<Tlhelp32.MODULEENTRY32W> modules = new ArrayList<Tlhelp32.MODULEENTRY32W>();
Win32Exception we = null;
try {
Tlhelp32.MODULEENTRY32W first = new Tlhelp32.MODULEENTRY32W();
modules.add(first);

if (!Kernel32.INSTANCE.Module32FirstW(snapshot, first)) {
throw new Win32Exception(Kernel32.INSTANCE.GetLastError());
}

List<Tlhelp32.MODULEENTRY32W> modules = new ArrayList<Tlhelp32.MODULEENTRY32W>();
modules.add(first);

Tlhelp32.MODULEENTRY32W next = new Tlhelp32.MODULEENTRY32W();
while (Kernel32.INSTANCE.Module32NextW(snapshot, next)) {
modules.add(next);
Expand All @@ -965,23 +1062,25 @@ public static List<Tlhelp32.MODULEENTRY32W> getModules(int processID) {
if (lastError != W32Errors.ERROR_SUCCESS && lastError != W32Errors.ERROR_NO_MORE_FILES) {
throw new Win32Exception(lastError);
}

return modules;
} catch (Win32Exception e) {
we = e;
throw we; // re-throw so finally block is executed
} finally {
if (snapshot != null) {
if (!Kernel32.INSTANCE.CloseHandle(snapshot)) {
Win32Exception e = new Win32Exception(Kernel32.INSTANCE.GetLastError());
if (we != null) {
e.addSuppressed(we);
}
try {
closeHandle(snapshot);
} catch(Win32Exception e) {
if (we == null) {
we = e;
} else {
we.addSuppressed(e);
}
}
}

if (we != null) {
throw we;
if (we != null) {
throw we;
}
}
return modules;
}
}
Loading