Michael Jung wrote:
Hello,
The original CryptAcquireContextA had some issues with memory management
in failure conditions, resulting in heap corruption under certain
cirumstances. I've reimplemented this function, checking behaviour against
Windows XP Prof. There is also a unit test included in the patch, which
tests CryptAcquireContext and successfully runs to completion on
both Windows XP and Wine. The patch looks worse than it is, since diff
believes to have found similarities between my new implementation and the
original CryptAcquireContextA.
Alexandre has already modified CryptAcquireContextA to fix the original problem, but I'll comment on the patch anyway.
Index: dlls/advapi32/crypt.c =================================================================== RCS file: /home/wine/wine/dlls/advapi32/crypt.c,v retrieving revision 1.39 diff -u -r1.39 crypt.c --- dlls/advapi32/crypt.c 16 Jul 2004 19:19:00 -0000 1.39 +++ dlls/advapi32/crypt.c 18 Jul 2004 22:55:19 -0000 @@ -239,6 +239,45 @@ #undef CRYPT_GetProvFunc #undef CRYPT_GetProvFuncOpt
+struct acquire_context_resources {
+ HKEY hKey;
+ PCHAR pszProvKeyName;
+ PCHAR pszTemp;
+ PCHAR pszImagePath;
+ PBYTE pSignature;
+};
+
+/******************************************************************************
+ * acquire_context_release_resources [Internal]
+ * + * Helper function for CryptAcquireContext. Releases the resources used by
+ * CryptAcquireContext, sets last error to error_code and returns TRUE if
+ * error_code == ERROR_SUCCESS, else FALSE.
+ * + * PARAMS
+ * resources [I] Pointer to an acquire_context_resources struct, which holds + * pointers to resources used by CryptAcquireContext
+ * error_code [I] Error code, which the "last error" is set to.
+ *
+ * RETURNS
+ * TRUE, iff error_code == ERROR_SUCCESS
+ * FALSE, otherwise
+ */
+inline BOOL acquire_context_release_resources(struct acquire_context_resources *resources, DWORD error_code)
+{
+ if (resources->hKey != (HKEY)INVALID_HANDLE_VALUE)
+ RegCloseKey(resources->hKey);
+ if (resources->pszProvKeyName != NULL)
+ HeapFree(GetProcessHeap(), 0, resources->pszProvKeyName);
+ if (resources->pszTemp != NULL)
+ HeapFree(GetProcessHeap(), 0, resources->pszTemp);
+ if (resources->pszImagePath != NULL)
+ HeapFree(GetProcessHeap(), 0, resources->pszImagePath);
+ if (resources->pSignature != NULL)
+ HeapFree(GetProcessHeap(), 0, resources->pSignature);
+ SetLastError(error_code);
+ return error_code == ERROR_SUCCESS;
+}
Two comments:
1. This seems like overkill - a new function *and* a new structure to go with it?
2. Use CRYPT_Free instead of HeapFree in case someone in the future changes the memory to routines to some non-Heap* (or remove the CRYPT memory routines altogether)
...
Your patch does highlight one issue though - we should be freeing all of the resource from one path, not multiple paths and the best way to do this is how it is currently done - by using a goto.
Maybe we could simplify the function by putting chunks into new functions, but certainly not a new function just to release local resources.
Rob
