Fix an SECItem leak in the new ECSignatureCreator class.

Fix an SECItem leak in the new ECSignatureCreator class.

R=rch@chromium.org
BUG=111317
TEST=ran drmemory on ECSignatureCreator.BasicTest, no longer reports leak


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

[Ryan Hamilton, 2012-01-31 00:10:47]

LGTM.  Thanks for the fix!

[commit-bot: I haz the power, 2012-01-31 14:53:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rnk@chromium.org/9302016/1

[commit-bot: I haz the power, 2012-01-31 14:53:55]

Presubmit check for 9302016-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: crypto/ec_signature_creator_nss.cc

[Reid Kleckner, 2012-01-31 14:58:32]

Adding Adam to get OWNERS approval.

[agl, 2012-01-31 15:40:55]

https://chromiumcodereview.appspot.com/9302016/diff/1/crypto/ec_signature_cre...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/9302016/diff/1/crypto/ec_signature_cre...
crypto/ec_signature_creator_nss.cc:96: SECITEM_FreeItem(result, PR_FALSE);
Thanks for catching this, but there's an underlying error here that should be
cleaned up.

|arena| is created and passed to |SignData|, but |SignData| doesn't use it, nor
can it. So |arena| should be removed, the first argument to SECITEM_AllocItem
should be "NULL /* no arena */". Lastly, the first argument to SignData should
be removed and then this line should be:

SECITEM_FreeItem(result, PR_TRUE /* also free |result| itself */);

[Ryan Hamilton, 2012-01-31 16:37:16]

https://chromiumcodereview.appspot.com/9302016/diff/1/crypto/ec_signature_cre...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/9302016/diff/1/crypto/ec_signature_cre...
crypto/ec_signature_creator_nss.cc:96: SECITEM_FreeItem(result, PR_FALSE);
On 2012/01/31 15:40:55, agl wrote:
> Thanks for catching this, but there's an underlying error here that should be
> cleaned up.
> 
> |arena| is created and passed to |SignData|, but |SignData| doesn't use it,
nor
> can it. So |arena| should be removed, the first argument to SECITEM_AllocItem
> should be "NULL /* no arena */". Lastly, the first argument to SignData should
> be removed and then this line should be:
> 
> SECITEM_FreeItem(result, PR_TRUE /* also free |result| itself */);

No kidding?  Thanks for catching this.  As per wtc's suggestion, I used
x509_util_nss.cc 's SignCertificate method as the basis for this implementation.
 When it invokes SignData, should it also not being using the arena?

[Reid Kleckner, 2012-01-31 16:56:40]

How about this?  I removed the arena and stack allocated result.

[agl, 2012-01-31 17:27:11]

LGTM, thanks!

[commit-bot: I haz the power, 2012-01-31 17:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rnk@chromium.org/9302016/5001

[wtc, 2012-01-31 18:43:57]

Patch Set 2 LGTM.  Nice catch and good cleanup!

https://chromiumcodereview.appspot.com/9302016/diff/5001/crypto/ec_signature_...
File crypto/ec_signature_creator_nss.cc (right):

https://chromiumcodereview.appspot.com/9302016/diff/5001/crypto/ec_signature_...
crypto/ec_signature_creator_nss.cc:90: SECITEM_FreeItem(&result, PR_FALSE /*
only free |result.data| */);

You can also just do
  PORT_Free(result.data);
to make this clearer.

[commit-bot: I haz the power, 2012-01-31 21:03:56]

Try job failure for 9302016-5001 (retry) (retry) on win_rel for step
"browser_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-01 17:19:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rnk@chromium.org/9302016/5001

[commit-bot: I haz the power, 2012-02-01 20:02:47]

Change committed as 120085