Dynamic memory management in CryptAcquireContext

[Michael Jung]

Alexandre,

I've noticed that you applied a modified version of my patch, which tries to 
fix a potential double free of the "temp" variable in dlls/advapi32/crypt.c's 
CryptAcquireContextA function. However, I think that the modified version 
does not fix the problem. Here is a code snippet from the current cvs 
versionc of crypt.c:

CRYPT_Free(temp);
  ...
  if (pProv->pFuncs->pCPAcquireContext(&pProv->hPrivate, (CHAR*)pszContainer, 
dwFlags, pProv->pVTable))
  {
    ...
    return TRUE;
  }
  /* FALLTHROUGH TO ERROR IF FALSE - CSP internal error! */
error:
  ...
  CRYPT_Free(temp);
  ...
  return FALSE;

If the CSP's CPAcquireContext function fails, which is quite common (e.g. 
because the user specified key container does not exist), the "temp" variable 
is still free'd twice.

I'm aware that my original patch was pretty ugly, due to a lot of redundancy, 
but I think this is due to fact that the CryptAcquireContext function seems 
not to be very well structured. I've submitted a patch two weeks ago, which 
basically was a reimplementation of CryptAcquireContext, but which was not 
committed (http://www.winehq.org/hypermail/wine-patches/2004/07/0213.html). 
Rob Shearman commented on this patch and felt that it is overkill to add both 
a helper function and a new struct type. Is this the reason the patch was not 
applied?

I would be glad to rework the patch, if you give me some hints about what you 
did not like. However, I would prefer to wait until James Hawkins got his 
a->w cross call cleanup patch committed, in order not to break his work.

Greetings,
Michael

[Alexandre Julliard]

Michael Jung  writes:
> If the CSP's CPAcquireContext function fails, which is quite common (e.g. 
> because the user specified key container does not exist), the "temp" variable 
> is still free'd twice.

Yes, the free needs to be moved inside the if, sorry about that.
> I'm aware that my original patch was pretty ugly, due to a lot of redundancy, 
> but I think this is due to fact that the CryptAcquireContext function seems 
> not to be very well structured. I've submitted a patch two weeks ago, which 
> basically was a reimplementation of CryptAcquireContext, but which was not 
> committed (http://www.winehq.org/hypermail/wine-patches/2004/07/0213.html). 
> Rob Shearman commented on this patch and felt that it is overkill to add both 
> a helper function and a new struct type. Is this the reason the patch was not 
> applied?

It's overkill and it's ugly. If a function needs such elaborate
cleanup it's probably because it's trying to do too many things, and
that needs to be fixed; splitting the cleanup part into a separate
function only makes things even more confusing.