From e7809ac26d9f1e5034600608d7818b0750eedbfa Mon Sep 17 00:00:00 2001 From: Lyor Goldstein Date: Tue, 1 Mar 2016 23:41:16 +0200 Subject: [PATCH] Make sure SID related memory is properly released once no longer required --- CHANGES.md | 1 + .../com/sun/jna/platform/win32/Advapi32.java | 54 +++---- .../sun/jna/platform/win32/Advapi32Util.java | 30 ++-- .../sun/jna/platform/win32/Advapi32Test.java | 136 +++++++++++------- 4 files changed, 136 insertions(+), 85 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index a8d5569e55..96d2dfd65a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -50,6 +50,7 @@ Bug Fixes * [#593](https://github.com/java-native-access/jna/pull/593): Improve binding of TypeLib bindings - [@matthiasblaesing](https://github.com/matthiasblaesing). * [#578](https://github.com/java-native-access/jna/pull/578): Fix COM CallbackHandlers, allow usage of VARIANTs directly in c.s.j.p.w.COM.util.ProxyObject and fix native memory leak in c.s.j.p.w.COM.util.ProxyObject - [@matthiasblaesing](https://github.com/matthiasblaesing) * [#601](https://github.com/java-native-access/jna/pull/601): Remove COMThread and COM initialization from objects and require callers to initialize COM themselves. Asserts are added to guard correct usage. - [@matthiasblaesing](https://github.com/matthiasblaesing). +* [#602] https://github.com/java-native-access/jna/pull/602): Make sure SID related memory is properly released once no longer required [@lgoldstein](https://github.com/lgoldstein) Release 4.2.1 ============= diff --git a/contrib/platform/src/com/sun/jna/platform/win32/Advapi32.java b/contrib/platform/src/com/sun/jna/platform/win32/Advapi32.java index 62da0fd004..520d2ebb2d 100755 --- a/contrib/platform/src/com/sun/jna/platform/win32/Advapi32.java +++ b/contrib/platform/src/com/sun/jna/platform/win32/Advapi32.java @@ -15,35 +15,33 @@ import com.sun.jna.Native; import com.sun.jna.Pointer; import com.sun.jna.Structure; -import com.sun.jna.WString; -import com.sun.jna.platform.win32.WinBase.SECURITY_ATTRIBUTES; -import com.sun.jna.platform.win32.WinBase.STARTUPINFO; import com.sun.jna.platform.win32.WinBase.FE_EXPORT_FUNC; import com.sun.jna.platform.win32.WinBase.FE_IMPORT_FUNC; import com.sun.jna.platform.win32.WinBase.PROCESS_INFORMATION; +import com.sun.jna.platform.win32.WinBase.SECURITY_ATTRIBUTES; +import com.sun.jna.platform.win32.WinBase.STARTUPINFO; +import com.sun.jna.platform.win32.WinDef.BOOLByReference; +import com.sun.jna.platform.win32.WinDef.DWORD; +import com.sun.jna.platform.win32.WinDef.DWORDByReference; +import com.sun.jna.platform.win32.WinDef.ULONG; +import com.sun.jna.platform.win32.WinNT.GENERIC_MAPPING; import com.sun.jna.platform.win32.WinNT.HANDLE; import com.sun.jna.platform.win32.WinNT.HANDLEByReference; +import com.sun.jna.platform.win32.WinNT.PRIVILEGE_SET; import com.sun.jna.platform.win32.WinNT.PSID; import com.sun.jna.platform.win32.WinNT.PSIDByReference; import com.sun.jna.platform.win32.WinReg.HKEY; import com.sun.jna.platform.win32.WinReg.HKEYByReference; +import com.sun.jna.platform.win32.Winsvc.ChangeServiceConfig2Info; import com.sun.jna.platform.win32.Winsvc.SC_HANDLE; import com.sun.jna.platform.win32.Winsvc.SERVICE_STATUS; import com.sun.jna.platform.win32.Winsvc.SERVICE_STATUS_PROCESS; -import com.sun.jna.platform.win32.Winsvc.ChangeServiceConfig2Info; import com.sun.jna.ptr.IntByReference; import com.sun.jna.ptr.LongByReference; import com.sun.jna.ptr.PointerByReference; import com.sun.jna.win32.StdCallLibrary; import com.sun.jna.win32.W32APIOptions; -import static com.sun.jna.platform.win32.WinDef.BOOLByReference; -import static com.sun.jna.platform.win32.WinDef.DWORD; -import static com.sun.jna.platform.win32.WinDef.DWORDByReference; -import static com.sun.jna.platform.win32.WinDef.ULONG; -import static com.sun.jna.platform.win32.WinNT.GENERIC_MAPPING; -import static com.sun.jna.platform.win32.WinNT.PRIVILEGE_SET; - /** * Advapi32.dll Interface. * @@ -185,7 +183,6 @@ boolean LookupAccountSid(String lpSystemName, PSID Sid, /** * Convert a security identifier (SID) to a string format suitable for * display, storage, or transmission. - * http://msdn.microsoft.com/en-us/library/aa376399(VS.85).aspx * * @param Sid * The SID structure to be converted. @@ -193,20 +190,25 @@ boolean LookupAccountSid(String lpSystemName, PSID Sid, * Pointer to a variable that receives a pointer to a * null-terminated SID string. To free the returned buffer, call * the LocalFree function. - * @return True if the function was successful, False otherwise. + * @return {@code true} if the function was successful - call {@code GetLastError()} + * to check failure reason + * @see ConvertSidToStringSid */ boolean ConvertSidToStringSid(PSID Sid, PointerByReference StringSid); /** * Convert a string-format security identifier (SID) into a valid, * functional SID. - * http://msdn.microsoft.com/en-us/library/aa376402(VS.85).aspx + * * * @param StringSid * The string-format SID to convert. * @param Sid - * Receives a pointer to the converted SID. - * @return True if the function was successful, False otherwise. + * Receives a pointer to the converted SID. To free the returned buffer, call + * the LocalFree function. + * @return {@code true} if the function was successful - call {@code GetLastError()} + * to check failure reason + * @see ConvertStringSidToSid */ boolean ConvertStringSidToSid(String StringSid, PSIDByReference Sid); @@ -1231,30 +1233,30 @@ boolean ReadEventLog(HANDLE hEventLog, int dwReadFlags, */ public boolean ChangeServiceConfig2(SC_HANDLE hService, int dwInfoLevel, ChangeServiceConfig2Info lpInfo); - + /** * Retrieves the optional configuration parameters of the specified service. - * + * * @param hService - * A handle to the service. This handle is returned by the OpenService or - * CreateService function and must have the SERVICE_QUERY_CONFIG access right. For + * A handle to the service. This handle is returned by the OpenService or + * CreateService function and must have the SERVICE_QUERY_CONFIG access right. For * more information, see Service Security and Access Rights. * @param dwInfoLevel * The configuration information to be queried. * @param lpBuffer - * A pointer to the buffer that receives the service configuration information. The + * A pointer to the buffer that receives the service configuration information. The * format of this data depends on the value of the dwInfoLevel parameter. - * The maximum size of this array is 8K bytes. To determine the required size, - * specify NULL for this parameter and 0 for the cbBufSize parameter. The function - * fails and GetLastError returns ERROR_INSUFFICIENT_BUFFER. The pcbBytesNeeded + * The maximum size of this array is 8K bytes. To determine the required size, + * specify NULL for this parameter and 0 for the cbBufSize parameter. The function + * fails and GetLastError returns ERROR_INSUFFICIENT_BUFFER. The pcbBytesNeeded * parameter receives the needed size. * @param cbBufSize * The size of the structure pointed to by the lpBuffer parameter, in bytes. * @param pcbBytesNeeded - * A pointer to a variable that receives the number of bytes required to store the + * A pointer to a variable that receives the number of bytes required to store the * configuration information, if the function fails with ERROR_INSUFFICIENT_BUFFER. * @return If the function succeeds, the return value is nonzero. - * If the function fails, the return value is zero. To get extended error information, + * If the function fails, the return value is zero. To get extended error information, * call GetLastError. */ public boolean QueryServiceConfig2(SC_HANDLE hService, int dwInfoLevel, diff --git a/contrib/platform/src/com/sun/jna/platform/win32/Advapi32Util.java b/contrib/platform/src/com/sun/jna/platform/win32/Advapi32Util.java index 81d707a25f..7103cf51eb 100755 --- a/contrib/platform/src/com/sun/jna/platform/win32/Advapi32Util.java +++ b/contrib/platform/src/com/sun/jna/platform/win32/Advapi32Util.java @@ -296,9 +296,13 @@ public static String convertSidToStringSid(PSID sid) { if (!Advapi32.INSTANCE.ConvertSidToStringSid(sid, stringSid)) { throw new Win32Exception(Kernel32.INSTANCE.GetLastError()); } - String result = stringSid.getValue().getWideString(0); - Kernel32.INSTANCE.LocalFree(stringSid.getValue()); - return result; + + Pointer ptr = stringSid.getValue(); + try { + return ptr.getWideString(0); + } finally { + Kernel32.INSTANCE.LocalFree(ptr); + } } /** @@ -314,7 +318,13 @@ public static byte[] convertStringSidToSid(String sidString) { if (!Advapi32.INSTANCE.ConvertStringSidToSid(sidString, pSID)) { throw new Win32Exception(Kernel32.INSTANCE.GetLastError()); } - return pSID.getValue().getBytes(); + + PSID value = pSID.getValue(); + try { + return value.getBytes(); + } finally { + Kernel32.INSTANCE.LocalFree(value.getPointer()); + } } /** @@ -332,8 +342,13 @@ public static boolean isWellKnownSid(String sidString, int wellKnownSidType) { if (!Advapi32.INSTANCE.ConvertStringSidToSid(sidString, pSID)) { throw new Win32Exception(Kernel32.INSTANCE.GetLastError()); } - return Advapi32.INSTANCE.IsWellKnownSid(pSID.getValue(), - wellKnownSidType); + + PSID value = pSID.getValue(); + try { + return Advapi32.INSTANCE.IsWellKnownSid(value, wellKnownSidType); + } finally { + Kernel32.INSTANCE.LocalFree(value.getPointer()); + } } /** @@ -372,8 +387,7 @@ public static Account getAccountBySid(String sidString) { * @return Account. */ public static Account getAccountBySid(String systemName, String sidString) { - return getAccountBySid(systemName, new PSID( - convertStringSidToSid(sidString))); + return getAccountBySid(systemName, new PSID(convertStringSidToSid(sidString))); } /** diff --git a/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java b/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java index 17666b6b3b..ceeb5cf838 100755 --- a/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java +++ b/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java @@ -137,60 +137,85 @@ public void testIsValidSid() { String sidString = EVERYONE; PSIDByReference sid = new PSIDByReference(); assertTrue("SID conversion failed", Advapi32.INSTANCE.ConvertStringSidToSid(sidString, sid)); - assertTrue("Converted SID not valid: " + sid.getValue(), Advapi32.INSTANCE.IsValidSid(sid.getValue())); - int sidLength = Advapi32.INSTANCE.GetLengthSid(sid.getValue()); - assertTrue(sidLength > 0); - assertTrue(Advapi32.INSTANCE.IsValidSid(sid.getValue())); + + PSID value = sid.getValue(); + try { + assertTrue("Converted SID not valid: " + value, Advapi32.INSTANCE.IsValidSid(value)); + int sidLength = Advapi32.INSTANCE.GetLengthSid(value); + assertTrue("Non positive sid length", sidLength > 0); + assertTrue("Invalid sid", Advapi32.INSTANCE.IsValidSid(value)); + } finally { + assertNull("Failed to release SID", Kernel32.INSTANCE.LocalFree(value.getPointer())); + } } public void testGetSidLength() { String sidString = EVERYONE; PSIDByReference sid = new PSIDByReference(); assertTrue("SID conversion failed", Advapi32.INSTANCE.ConvertStringSidToSid(sidString, sid)); - assertEquals("Wrong SID lenght", 12, Advapi32.INSTANCE.GetLengthSid(sid.getValue())); + + PSID value = sid.getValue(); + try { + assertEquals("Wrong SID length", 12, Advapi32.INSTANCE.GetLengthSid(value)); + } finally { + assertNull("Failed to free SID", Kernel32.INSTANCE.LocalFree(value.getPointer())); + } } public void testLookupAccountSid() { // get SID bytes String sidString = EVERYONE; PSIDByReference sid = new PSIDByReference(); - assertTrue(Advapi32.INSTANCE.ConvertStringSidToSid(sidString, sid)); - int sidLength = Advapi32.INSTANCE.GetLengthSid(sid.getValue()); - assertTrue(sidLength > 0); - // lookup account - IntByReference cchName = new IntByReference(); - IntByReference cchReferencedDomainName = new IntByReference(); - PointerByReference peUse = new PointerByReference(); - assertFalse(Advapi32.INSTANCE.LookupAccountSid(null, sid.getValue(), - null, cchName, null, cchReferencedDomainName, peUse)); - assertEquals(W32Errors.ERROR_INSUFFICIENT_BUFFER, Kernel32.INSTANCE.GetLastError()); - assertTrue(cchName.getValue() > 0); - assertTrue(cchReferencedDomainName.getValue() > 0); - char[] referencedDomainName = new char[cchReferencedDomainName.getValue()]; - char[] name = new char[cchName.getValue()]; - assertTrue(Advapi32.INSTANCE.LookupAccountSid(null, sid.getValue(), - name, cchName, referencedDomainName, cchReferencedDomainName, peUse)); - assertEquals(5, peUse.getPointer().getInt(0)); // SidTypeWellKnownGroup - String nameString = Native.toString(name); - String referencedDomainNameString = Native.toString(referencedDomainName); - assertTrue(nameString.length() > 0); - assertEquals("Everyone", nameString); - assertTrue(referencedDomainNameString.length() == 0); - assertEquals(null, Kernel32.INSTANCE.LocalFree(sid.getValue().getPointer())); + assertTrue("Failed to create sid", Advapi32.INSTANCE.ConvertStringSidToSid(sidString, sid)); + + PSID value = sid.getValue(); + try { + int sidLength = Advapi32.INSTANCE.GetLengthSid(value); + assertTrue("Non-positive sid length", sidLength > 0); + // lookup account + IntByReference cchName = new IntByReference(); + IntByReference cchReferencedDomainName = new IntByReference(); + PointerByReference peUse = new PointerByReference(); + assertFalse(Advapi32.INSTANCE.LookupAccountSid(null, value, + null, cchName, null, cchReferencedDomainName, peUse)); + assertEquals(W32Errors.ERROR_INSUFFICIENT_BUFFER, Kernel32.INSTANCE.GetLastError()); + assertTrue(cchName.getValue() > 0); + assertTrue(cchReferencedDomainName.getValue() > 0); + char[] referencedDomainName = new char[cchReferencedDomainName.getValue()]; + char[] name = new char[cchName.getValue()]; + assertTrue(Advapi32.INSTANCE.LookupAccountSid(null, value, + name, cchName, referencedDomainName, cchReferencedDomainName, peUse)); + assertEquals(5, peUse.getPointer().getInt(0)); // SidTypeWellKnownGroup + String nameString = Native.toString(name); + String referencedDomainNameString = Native.toString(referencedDomainName); + assertTrue(nameString.length() > 0); + assertEquals("Everyone", nameString); + assertTrue(referencedDomainNameString.length() == 0); + } finally { + assertNull("Failed to release sid", Kernel32.INSTANCE.LocalFree(value.getPointer())); + } } public void testConvertSid() { String sidString = EVERYONE; PSIDByReference sid = new PSIDByReference(); - assertTrue(Advapi32.INSTANCE.ConvertStringSidToSid( - sidString, sid)); - PointerByReference convertedSidStringPtr = new PointerByReference(); - assertTrue(Advapi32.INSTANCE.ConvertSidToStringSid( - sid.getValue(), convertedSidStringPtr)); - String convertedSidString = convertedSidStringPtr.getValue().getWideString(0); - assertEquals(convertedSidString, sidString); - assertEquals(null, Kernel32.INSTANCE.LocalFree(convertedSidStringPtr.getValue())); - assertEquals(null, Kernel32.INSTANCE.LocalFree(sid.getValue().getPointer())); + assertTrue("Failed to convert SID string", Advapi32.INSTANCE.ConvertStringSidToSid(sidString, sid)); + + PSID value = sid.getValue(); + try { + PointerByReference convertedSidStringPtr = new PointerByReference(); + assertTrue("Failed to convert SID string", Advapi32.INSTANCE.ConvertSidToStringSid(value, convertedSidStringPtr)); + + Pointer conv = convertedSidStringPtr.getValue(); + try { + String convertedSidString = conv.getWideString(0); + assertEquals("Mismatched SID string", convertedSidString, sidString); + } finally { + assertNull("Failed to release string value", Kernel32.INSTANCE.LocalFree(conv)); + } + } finally { + assertNull("Failed to release sid", Kernel32.INSTANCE.LocalFree(value.getPointer())); + } } public void testLogonUser() { @@ -594,26 +619,35 @@ public void testRegQueryInfoKey() { public void testIsWellKnownSid() { String sidString = EVERYONE; PSIDByReference sid = new PSIDByReference(); - assertTrue(Advapi32.INSTANCE.ConvertStringSidToSid(sidString, sid)); - assertTrue(Advapi32.INSTANCE.IsWellKnownSid(sid.getValue(), - WELL_KNOWN_SID_TYPE.WinWorldSid)); - assertFalse(Advapi32.INSTANCE.IsWellKnownSid(sid.getValue(), - WELL_KNOWN_SID_TYPE.WinAccountAdministratorSid)); + assertTrue("sid conversion failed", Advapi32.INSTANCE.ConvertStringSidToSid(sidString, sid)); + + PSID value = sid.getValue(); + try { + assertTrue("Not a world sid", Advapi32.INSTANCE.IsWellKnownSid(value, WELL_KNOWN_SID_TYPE.WinWorldSid)); + assertFalse("Unexpected admin sid", Advapi32.INSTANCE.IsWellKnownSid(value, WELL_KNOWN_SID_TYPE.WinAccountAdministratorSid)); + } finally { + assertNull("Failed to release sid", Kernel32.INSTANCE.LocalFree(value.getPointer())); + } } public void testCreateWellKnownSid() { PSID pSid = new PSID(WinNT.SECURITY_MAX_SID_SIZE); IntByReference cbSid = new IntByReference(WinNT.SECURITY_MAX_SID_SIZE); - assertTrue(Advapi32.INSTANCE.CreateWellKnownSid(WELL_KNOWN_SID_TYPE.WinWorldSid, - null, pSid, cbSid)); - assertTrue(Advapi32.INSTANCE.IsWellKnownSid(pSid, - WELL_KNOWN_SID_TYPE.WinWorldSid)); - assertTrue(cbSid.getValue() <= WinNT.SECURITY_MAX_SID_SIZE); + assertTrue("Failed to create well-known SID", + Advapi32.INSTANCE.CreateWellKnownSid(WELL_KNOWN_SID_TYPE.WinWorldSid, null, pSid, cbSid)); + assertTrue("Not recognized as well-known SID", + Advapi32.INSTANCE.IsWellKnownSid(pSid, WELL_KNOWN_SID_TYPE.WinWorldSid)); + assertTrue("Invalid SID size", cbSid.getValue() <= WinNT.SECURITY_MAX_SID_SIZE); PointerByReference convertedSidStringPtr = new PointerByReference(); - assertTrue(Advapi32.INSTANCE.ConvertSidToStringSid( - pSid, convertedSidStringPtr)); - String convertedSidString = convertedSidStringPtr.getValue().getWideString(0); - assertEquals(EVERYONE, convertedSidString); + assertTrue("Failed to convert SID", Advapi32.INSTANCE.ConvertSidToStringSid(pSid, convertedSidStringPtr)); + + Pointer conv = convertedSidStringPtr.getValue(); + try { + String convertedSidString = conv.getWideString(0); + assertEquals("Mismatched SID string", EVERYONE, convertedSidString); + } finally { + assertNull("Failed to release string", Kernel32.INSTANCE.LocalFree(conv)); + } } public void testOpenEventLog() {