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:
- 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