Fix nits in SecureHash.

Fix nits in SecureHash.

R=agl@chromium.org,rsleevi@chromium.org
BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170308

[wtc, 2012-11-28 20:49:30]

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc
File crypto/secure_hash_default.cc (right):

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc...
crypto/secure_hash_default.cc:27: memset(&ctx_, 0, sizeof(ctx_));

This matches what the SecureHashSHA256OpenSSL destructor does:

  virtual ~SecureHashSHA256OpenSSL() {
    OPENSSL_cleanse(&ctx_, sizeof(ctx_));
  }

[agl, 2012-11-28 21:25:30]

LGTM, depending on how you feel about the compiler niggle.

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc
File crypto/secure_hash_default.cc (right):

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc...
crypto/secure_hash_default.cc:27: memset(&ctx_, 0, sizeof(ctx_));
On 2012/11/28 20:49:30, wtc wrote:
> 
> This matches what the SecureHashSHA256OpenSSL destructor does:
> 
>   virtual ~SecureHashSHA256OpenSSL() {
>     OPENSSL_cleanse(&ctx_, sizeof(ctx_));
>   }

I believe that there is a significant chance that compilers will remove this:
http://www.viva64.com/en/b/0178/

[Ryan Sleevi, 2012-11-28 22:17:08]

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc
File crypto/secure_hash_default.cc (right):

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc...
crypto/secure_hash_default.cc:27: memset(&ctx_, 0, sizeof(ctx_));
On 2012/11/28 21:25:30, agl wrote:
> On 2012/11/28 20:49:30, wtc wrote:
> > 
> > This matches what the SecureHashSHA256OpenSSL destructor does:
> > 
> >   virtual ~SecureHashSHA256OpenSSL() {
> >     OPENSSL_cleanse(&ctx_, sizeof(ctx_));
> >   }
> 
> I believe that there is a significant chance that compilers will remove this:
> http://www.viva64.com/en/b/0178/

Agreed. It's gonna be a nop on -O2 and higher.

See
http://cwe.mitre.org/data/definitions/14.html

[wtc, 2012-11-28 22:39:36]

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc
File crypto/secure_hash_default.cc (right):

https://codereview.chromium.org/11299235/diff/1/crypto/secure_hash_default.cc...
crypto/secure_hash_default.cc:27: memset(&ctx_, 0, sizeof(ctx_));

Thanks for pointing this out. We should add a function to
src/base or NSS to do this.

OPENSSL_cleanse seems to rely on the use of a global variable
(cleanse_ctr in crypto/mem_clr.c) to make the compiler think
the function cannot be optimized away. I wonder if there is
another simple way to prevent compiler from removing the
function call. (On Windows we can just call the Windows
SecureZeroMemory function.)

[commit-bot: I haz the power, 2012-11-29 19:38:11]

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

[commit-bot: I haz the power, 2012-11-29 19:55:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 20:39:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/11299235/11001

[commit-bot: I haz the power, 2012-11-30 00:37:05]

Change committed as 170308