Separate the initialization code in the constructor of HMAC class into Init f...

[Replaced by http://codereview.chromium.org/113001 ]

Separate the key setting code in the constructor of HMAC class into the Init method.

Overload the Init method for char* and std::string.

Add DCHECKs to the destruction methods in ~HMAC in hmac_win.cc.

BUG=2297

[tyoshino-do-not-use, 2009-04-30 17:37:19]

Hi Wan,

I'd like you to review my change on the HMAC class files.

This is a response to this BUG entry.
http://code.google.com/p/chromium/issues/detail?id=2297

Thanks,

[wtc, 2009-04-30 21:38:43]

Thanks a lot for writing this changelist.  It looks
good, but I'd like to suggest renaming the SetKey method.
Also note the bug about DCHECKs in hmac_win.cc.

http://codereview.chromium.org/88062/diff/2001/2002
File base/hmac.h (right):

http://codereview.chromium.org/88062/diff/2001/2002#newcode33
Line 33: // Initializes this instance using |key|.
Nit: Please add a blank line above this line to separate
method declarations.

We should name this method "Init" or "SignInit".  "Init"
implies that it can only be called once, so we can assume
there's no previous key.  "SignInit" implies that we need
to call it before each Sign() operation, so we need to
deal with the previous key properly.

I would just name the method "Init" and assume that there's
no previous key.  We can make Init return false if it
detects that the object is already initialized.

http://codereview.chromium.org/88062/diff/2001/2002#newcode40
Line 40: // supplied to the SetKey. The HMAC is returned in |digest|, which
Nit: change "the SetKey" to "SetKey" or "the SetKey method".

http://codereview.chromium.org/88062/diff/2001/2004
File base/hmac_mac.cc (right):

http://codereview.chromium.org/88062/diff/2001/2004#newcode22
Line 22: // Zero out the previous key.
Nit: This shouldn't be necessary, especially if we name this
method "Init", because there should be no previous key.

http://codereview.chromium.org/88062/diff/2001/2003
File base/hmac_nss.cc (right):

http://codereview.chromium.org/88062/diff/2001/2003#newcode49
Line 49: base::EnsureNSSInit();
Nit: The base::EnsureNSSInit() call should also be moved to
the SetKey method.

http://codereview.chromium.org/88062/diff/2001/2007
File base/hmac_unittest.cc (right):

http://codereview.chromium.org/88062/diff/2001/2007#newcode55
Line 55: hmac.SetKey(kClientKey, kKeySize);
We should check the return value.  That's the whole point
of this bug (that the initialization may fail).

http://codereview.chromium.org/88062/diff/2001/2005
File base/hmac_win.cc (right):

http://codereview.chromium.org/88062/diff/2001/2005#newcode29
Line 29: // Release the context used for the previous key.
We can keep using the same context even if there is a previous
key.  So this should be removed.

http://codereview.chromium.org/88062/diff/2001/2005#newcode87
Line 87: DCHECK(CryptDestroyKey(plat_->hkey_));
This should be done as follows:
  BOOL ok;
  if (plat_->hkey_) {
    ok = CryptDestroyKey(plat_->hkey_);
    DCHECK(ok);
  }

We can't put the CryptDestroyKey call inside DCHECK
because CryptDestroyKey won't be called when DCHECK
is disabled (the default in Release builds).

http://codereview.chromium.org/88062/diff/2001/2006
File chrome/browser/safe_browsing/safe_browsing_util.cc (right):

http://codereview.chromium.org/88062/diff/2001/2006#newcode174
Line 174: hmac.SetKey(decoded_key);
Check the return value.

[tyoshino-do-not-use, 2009-05-01 19:56:44]

Thank you for review. I've addressed your comments.
Also, I tested this change on the Try Server. All passed.

http://codereview.chromium.org/88062/diff/2001/2002
File base/hmac.h (right):

http://codereview.chromium.org/88062/diff/2001/2002#newcode33
Line 33: // Initializes this instance using |key|.
On 2009/04/30 21:38:43, wtc wrote:
> Nit: Please add a blank line above this line to separate
> method declarations.
> 
> We should name this method "Init" or "SignInit".  "Init"
> implies that it can only be called once, so we can assume
> there's no previous key.  "SignInit" implies that we need
> to call it before each Sign() operation, so we need to
> deal with the previous key properly.
> 
> I would just name the method "Init" and assume that there's
> no previous key.  We can make Init return false if it
> detects that the object is already initialized.

Done.

http://codereview.chromium.org/88062/diff/2001/2002#newcode40
Line 40: // supplied to the SetKey. The HMAC is returned in |digest|, which
On 2009/04/30 21:38:43, wtc wrote:
> Nit: change "the SetKey" to "SetKey" or "the SetKey method".

Done.

http://codereview.chromium.org/88062/diff/2001/2004
File base/hmac_mac.cc (right):

http://codereview.chromium.org/88062/diff/2001/2004#newcode22
Line 22: // Zero out the previous key.
On 2009/04/30 21:38:43, wtc wrote:
> Nit: This shouldn't be necessary, especially if we name this
> method "Init", because there should be no previous key.

Done. Now I just do check that key_ is empty here.

http://codereview.chromium.org/88062/diff/2001/2003
File base/hmac_nss.cc (right):

http://codereview.chromium.org/88062/diff/2001/2003#newcode49
Line 49: base::EnsureNSSInit();
On 2009/04/30 21:38:43, wtc wrote:
> Nit: The base::EnsureNSSInit() call should also be moved to
> the SetKey method.

Done.

http://codereview.chromium.org/88062/diff/2001/2007
File base/hmac_unittest.cc (right):

http://codereview.chromium.org/88062/diff/2001/2007#newcode55
Line 55: hmac.SetKey(kClientKey, kKeySize);
On 2009/04/30 21:38:43, wtc wrote:
> We should check the return value.  That's the whole point
> of this bug (that the initialization may fail).

Oh, sorry. Done.

http://codereview.chromium.org/88062/diff/2001/2005
File base/hmac_win.cc (right):

http://codereview.chromium.org/88062/diff/2001/2005#newcode29
Line 29: // Release the context used for the previous key.
On 2009/04/30 21:38:43, wtc wrote:
> We can keep using the same context even if there is a previous
> key.  So this should be removed.

Done. Because we renamed this method to Init, I've put code to check that no
object has been initialized here instead.

http://codereview.chromium.org/88062/diff/2001/2005#newcode87
Line 87: DCHECK(CryptDestroyKey(plat_->hkey_));
On 2009/04/30 21:38:43, wtc wrote:
> This should be done as follows:
>   BOOL ok;
>   if (plat_->hkey_) {
>     ok = CryptDestroyKey(plat_->hkey_);
>     DCHECK(ok);
>   }
> 
> We can't put the CryptDestroyKey call inside DCHECK
> because CryptDestroyKey won't be called when DCHECK
> is disabled (the default in Release builds).

Done.

http://codereview.chromium.org/88062/diff/2001/2006
File chrome/browser/safe_browsing/safe_browsing_util.cc (right):

http://codereview.chromium.org/88062/diff/2001/2006#newcode174
Line 174: hmac.SetKey(decoded_key);
On 2009/04/30 21:38:43, wtc wrote:
> Check the return value.

Done.

[wtc, 2009-05-04 23:07:34]

LGTM.  Please remember to update the CL's description,
which still mentions the "SetKey" function, before you
check in.

I have just two nits below.  After you fix the nits,
you can check in without further review.  Thanks!

http://codereview.chromium.org/88062/diff/3022/2034
File base/hmac.h (right):

http://codereview.chromium.org/88062/diff/3022/2034#newcode42
Line 42: // Calculates the HMAC for the message in |data| using the algorithm
and key
Nit: to match the current implementation, this comment should
say:
  ... using the algorithm supplied to the constructor and
  the key supplied to the Init method. ...

Alternatively, we can move the hash_alg parameter from the
constructor the the Init method.

http://codereview.chromium.org/88062/diff/3022/2035
File base/hmac_nss.cc (right):

http://codereview.chromium.org/88062/diff/3022/2035#newcode59
Line 59: if (plat_->slot_.get() != NULL || plat_->slot_.get() != NULL) {
Nit: remove != NULL from these tests because the existing
code in this file perform a null pointer check using
  if (!plat_->slot_.get()) {

In general the new code we add should follow the prevalent
style in the file.

[tyoshino-do-not-use, 2009-05-05 20:23:27]

Thank you. Addressed your inlined comments and updated the CL description, and
then run gcl try on the final patch. Success.

Could you check in this on behalf of me? I don't have privilege to commit
changes, yet. Thanks.

http://codereview.chromium.org/88062/diff/3022/2034
File base/hmac.h (right):

http://codereview.chromium.org/88062/diff/3022/2034#newcode42
Line 42: // Calculates the HMAC for the message in |data| using the algorithm
and key
On 2009/05/04 23:07:34, wtc wrote:
> Nit: to match the current implementation, this comment should
> say:
>   ... using the algorithm supplied to the constructor and
>   the key supplied to the Init method. ...
> 
> Alternatively, we can move the hash_alg parameter from the
> constructor the the Init method.

Done.

http://codereview.chromium.org/88062/diff/3022/2035
File base/hmac_nss.cc (right):

http://codereview.chromium.org/88062/diff/3022/2035#newcode59
Line 59: if (plat_->slot_.get() != NULL || plat_->slot_.get() != NULL) {
On 2009/05/04 23:07:34, wtc wrote:
> Nit: remove != NULL from these tests because the existing
> code in this file perform a null pointer check using
>   if (!plat_->slot_.get()) {
> 

Done.

> In general the new code we add should follow the prevalent
> style in the file.

OK.

http://codereview.chromium.org/88062/diff/4001/5004
File base/hmac_win.cc (right):

http://codereview.chromium.org/88062/diff/4001/5004#newcode31
Line 31: if (plat_->provider_ != NULL || plat_->hkey_ != NULL) {
Fixed here, too.

[tyoshino-do-not-use, 2009-05-06 00:01:44]

Thank you for submitting.