Reimplementation of CryptAcquireContextA

[Michael Jung]

Am Montag, 19. Juli 2004 02:47 schrieb 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.

...

> 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

Hi Rob,

The original CryptAcquireContextA had a couple of memory management issues
and Alexandre's patch only takes care of one. Furthermore, the original
implementation does not comply to Windows behaviour regarding error
reporting. The unit test given in the patch succesfully runs to completion on
both Windows XP Professional and Wine, but only with the new
CryptAcquireContext.

The function and the structure are used to keep the CryptAcquireContext
function body more readable. As you pointed out, this could also be achieved
with goto's and a labeled cleanup section (Though not as compact, since you
still would have to do a "SetLastError(..); goto error;", which requires an
additional pair of curly braces ;) ). Personally I don't like goto's, but
this is perhaps a position too academic. So if the wine community favours the
goto approach, I will be happy to modify the code.

The patch uses the HeapAlloc function only for memory blocks, which are
completely memory managed inside CryptAcquireContext. This means that the
implementation of CRYPT_Free is irrelevant here. As far as I understand it,
HeapAlloc is the way to go in new code. Am I correct here?

Since I'm new to wine, I don't have a firm understanding of the coding
conventions. Could some more people comment on the given patch, please? This
would help me to get a better picture.

Thanks and greetings,
Michael