Reimplementation of CryptAcquireContextA
Robert Shearman
rob at codeweavers.com
Sun Jul 18 19:47:43 CDT 2004
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
More information about the wine-devel
mailing list