Reimplementation of CryptAcquireContextA

[Robert Shearman]

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