Implement signature_creator_mac

Implement signature_creator_mac. Also moved CSSM memory functions to a common shared location.

BUG=20669
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28135

[hawk, 2009-10-03 01:11:25]

http://codereview.chromium.org/259026/diff/1/3
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/259026/diff/1/3#newcode9
Line 9: #include "base/scoped_ptr.h"
Is this include needed?

http://codereview.chromium.org/259026/diff/1/7
File base/crypto/signature_creator_mac.cc (right):

http://codereview.chromium.org/259026/diff/1/7#newcode86
Line 86: free(sig.Data); // Release data alloc'd by CSSM
This free() call will be a problem if we switch to using TCMalloc on the Mac
(which is being discussed). sig.Data will have been allocated by the malloc() in
System.framework and will be freed by TCMalloc. You should allocate the memory
instead.

[wtc, 2009-10-05 18:33:26]

LGTM, except some extra blank lines and the memory allocation
issue with sig.Data that hawk pointed out.

http://codereview.chromium.org/259026/diff/1/3
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/259026/diff/1/3#newcode8
Line 8: 
Nit: delete this blank line.  Also scoped_ptr.h should
come before singleton.h.

http://codereview.chromium.org/259026/diff/1/3#newcode95
Line 95: 
Nit: delete one blank line here.

http://codereview.chromium.org/259026/diff/1/4
File base/crypto/cssm_init.h (right):

http://codereview.chromium.org/259026/diff/1/4#newcode9
Line 9: 
Nit: delete one blank line here.

http://codereview.chromium.org/259026/diff/1/7
File base/crypto/signature_creator_mac.cc (right):

http://codereview.chromium.org/259026/diff/1/7#newcode49
Line 49: 
Delete this blank line.

http://codereview.chromium.org/259026/diff/1/7#newcode57
Line 57: sig_handle_ = 0;
It's not necessary to set sig_handle_ to 0 in a destructor.
If you want to do it for style reasons, please also set
csp_handle_ to 0 below for consistency.

http://codereview.chromium.org/259026/diff/1/7#newcode77
Line 77: memset(&sig, 0, sizeof(CSSM_DATA)); // Allow CSSM allow memory;
Nit: The comment should read
  // Allow CSSM to allocate memory.

http://codereview.chromium.org/259026/diff/1/7#newcode86
Line 86: free(sig.Data); // Release data alloc'd by CSSM
On 2009/10/03 01:11:25, hawk wrote:
> This free() call will be a problem if we switch to using TCMalloc on the Mac
> (which is being discussed). sig.Data will have been allocated by the malloc()
in
> System.framework and will be freed by TCMalloc. You should allocate the memory
> instead.

hawk, are you sure sig.Data is allocated by the malloc()
in System.framework rather than our
kCssmMemoryFunctions.malloc_func() function?

Can we use kCssmMemoryFunctions.free_func() to free
sig.Data here?

[rafaelw, 2009-10-05 22:46:18]

http://codereview.chromium.org/259026/diff/1/3
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/259026/diff/1/3#newcode8
Line 8: 
On 2009/10/05 18:33:26, wtc wrote:
> Nit: delete this blank line.  Also scoped_ptr.h should
> come before singleton.h.

Done.

http://codereview.chromium.org/259026/diff/1/3#newcode9
Line 9: #include "base/scoped_ptr.h"
On 2009/10/03 01:11:25, hawk wrote:
> Is this include needed?

Removed.

http://codereview.chromium.org/259026/diff/1/3#newcode95
Line 95: 
On 2009/10/05 18:33:26, wtc wrote:
> Nit: delete one blank line here.

Done.

http://codereview.chromium.org/259026/diff/1/4
File base/crypto/cssm_init.h (right):

http://codereview.chromium.org/259026/diff/1/4#newcode9
Line 9: 
On 2009/10/05 18:33:26, wtc wrote:
> Nit: delete one blank line here.

Done.

http://codereview.chromium.org/259026/diff/1/7
File base/crypto/signature_creator_mac.cc (right):

http://codereview.chromium.org/259026/diff/1/7#newcode49
Line 49: 
On 2009/10/05 18:33:26, wtc wrote:
> Delete this blank line.

Done.

http://codereview.chromium.org/259026/diff/1/7#newcode57
Line 57: sig_handle_ = 0;
On 2009/10/05 18:33:26, wtc wrote:
> It's not necessary to set sig_handle_ to 0 in a destructor.
> If you want to do it for style reasons, please also set
> csp_handle_ to 0 below for consistency.

Done.

http://codereview.chromium.org/259026/diff/1/7#newcode77
Line 77: memset(&sig, 0, sizeof(CSSM_DATA)); // Allow CSSM allow memory;
On 2009/10/05 18:33:26, wtc wrote:
> Nit: The comment should read
>   // Allow CSSM to allocate memory.

Done.

http://codereview.chromium.org/259026/diff/1/7#newcode86
Line 86: free(sig.Data); // Release data alloc'd by CSSM
On 2009/10/05 18:33:26, wtc wrote:
> On 2009/10/03 01:11:25, hawk wrote:
> > This free() call will be a problem if we switch to using TCMalloc on the Mac
> > (which is being discussed). sig.Data will have been allocated by the
malloc()
> in
> > System.framework and will be freed by TCMalloc. You should allocate the
memory
> > instead.
> 
> hawk, are you sure sig.Data is allocated by the malloc()
> in System.framework rather than our
> kCssmMemoryFunctions.malloc_func() function?
> 
> Can we use kCssmMemoryFunctions.free_func() to free
> sig.Data here?

I'm replaced this call with kCssmMemoryFunctions.free_func(). Hawk, please weigh
in. I was assuming that cssm would be using our
kCssmMemoryFunctions.malloc_func.

[hawk, 2009-10-05 22:50:14]

LGTM

http://codereview.chromium.org/259026/diff/1/7
File base/crypto/signature_creator_mac.cc (right):

http://codereview.chromium.org/259026/diff/1/7#newcode86
Line 86: free(sig.Data); // Release data alloc'd by CSSM
> I'm replaced this call with kCssmMemoryFunctions.free_func(). Hawk, please
weigh
> in. I was assuming that cssm would be using our
> kCssmMemoryFunctions.malloc_func.

That should be fine.

[wtc, 2009-10-06 01:26:35]

http://codereview.chromium.org/259026/diff/5001/4007
File base/crypto/signature_creator_mac.cc (right):

http://codereview.chromium.org/259026/diff/5001/4007#newcode84
Line 84: kCssmMemoryFunctions.free_func(sig.Data); // Release data alloc'd by
CSSM
Did you test this?  Don't you need to pass NULL as the
second (alloc_ref) argument to free_func()?

[rafaelw, 2009-10-06 02:42:54]

http://codereview.chromium.org/259026/diff/5001/4007
File base/crypto/signature_creator_mac.cc (right):

http://codereview.chromium.org/259026/diff/5001/4007#newcode84
Line 84: kCssmMemoryFunctions.free_func(sig.Data); // Release data alloc'd by
CSSM
On 2009/10/06 01:26:35, wtc wrote:
> Did you test this?  Don't you need to pass NULL as the
> second (alloc_ref) argument to free_func()?

Right you are. I didn't test it yet (I wasn't sitting in front of my mac today
and remoting to it is useless). Corrected.