Added serialization to SecureHash.

Added serialization to SecureHash.

These will be used to resume hash generation when a download is interrupted and later resumed.

BUG=None.
TEST=SecureHashTest.TestSerialization


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

[ahendrickson, 2011-11-18 17:51:43]

Hi Randy,

I'm working on a unit test, but this is the first pass.

[Randy Smith (Not in Mondays), 2011-11-18 19:01:47]

Basic architecture LGTM, modulo the question below about maybe making a variant
of the Create() method that takes a Pickle.  Details of the implementation will
need to be reviewed by someone who knows the crypto code (which I presume was
your intent, but the initial message wasn't clear that I was doing a first pass
before review by OWNERS, so I wanted to be explicit about it).

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash.h
File crypto/secure_hash.h (right):

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash.h#newcode35
crypto/secure_hash.h:35: // Restore the context that was saved earlier.
Maybe worthwhile indicating that it will wipe out any in-progress context?

I find myself wondering if this could be replaced by a constructor taking a
Pickle*, but my guess is that that goes against the way Pickle is used in most
places, so it's probably not a good idea.  A related thing that might or might
not be a good idea would be to make a version of the Create() method that takes
a Pickle and automatically creates the type of SecureHash that it represents;
something to chat with the crypto owners about.

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash_unittest.cc
File crypto/secure_hash_unittest.cc (right):

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash_unittest....
crypto/secure_hash_unittest.cc:51: crypto::SecureHash::SHA256));
Don't we want to have a similar test for the SSL hash type?

[ahendrickson, 2011-11-18 19:47:00]

Now open for wider review.

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash.h
File crypto/secure_hash.h (right):

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash.h#newcode35
crypto/secure_hash.h:35: // Restore the context that was saved earlier.
On 2011/11/18 19:01:47, rdsmith wrote:
> Maybe worthwhile indicating that it will wipe out any in-progress context?
> 
> I find myself wondering if this could be replaced by a constructor taking a
> Pickle*, but my guess is that that goes against the way Pickle is used in most
> places, so it's probably not a good idea.  A related thing that might or might
> not be a good idea would be to make a version of the Create() method that
takes
> a Pickle and automatically creates the type of SecureHash that it represents;
> something to chat with the crypto owners about.

I don't believe that we can currently have both NSS and OpenSSL hashes in the
same build, although it looks like we're on the way to supporting both.  The
Create() code takes the same parameter in both implementations, it's just that
only one of them is built at a time.  See crbug 62803.

There is a flag in build/common.gyp called use_openssl.

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash_unittest.cc
File crypto/secure_hash_unittest.cc (right):

http://codereview.chromium.org/8588057/diff/3001/crypto/secure_hash_unittest....
crypto/secure_hash_unittest.cc:51: crypto::SecureHash::SHA256));
On 2011/11/18 19:01:47, rdsmith wrote:
> Don't we want to have a similar test for the SSL hash type?

I believe one or the other hash types (NSS or OpenSSL) will be build depending
on GYP flags, so this test will work with either one.

[ahendrickson, 2011-11-18 19:49:46]

One note:  The fallback position for failing to deserialize (in terms of
downloads) is that we will have to read the file from the beginning to create
the hash up to the interruption point.

[agl, 2011-11-18 19:57:02]

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc
File crypto/secure_hash_default.cc (right):

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:44: const char* SecureHashSHA256NSS::kDescriptor =
"NSS";
Move it out of the class and do:

static const char kSHA256Descriptor[] = "NSS";

to save a relocation record.

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:50: if (!pickle->WriteInt(kSecureHashVersion))
if (!pickle->WriteInt(kSecureHashVersion) ||
    !pickle->WriteString(kDescriptor) ||
    !pickle->WriteBytes(&ctx_, sizeof(ctx_))) {
  return false;
}

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:60: bool SecureHashSHA256NSS::Deserialize(Pickle*
pickle) {
this interface (specifically the lack of a void* iterator param) precludes the
serialized hash being part of a larger Pickle.

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:79: char data_buffer[sizeof(ctx_)] = "";
Pickle doesn't need a buffer to write it. It sets |data| to point into the
underlying bytes.

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_openssl.cc
File crypto/secure_hash_openssl.cc (right):

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_openssl.cc#n...
crypto/secure_hash_openssl.cc:20: static const int kSecureHashVersion = 1;
all the same comments apply to this file.

[ahendrickson, 2011-11-18 21:23:09]

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc
File crypto/secure_hash_default.cc (right):

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:44: const char* SecureHashSHA256NSS::kDescriptor =
"NSS";
On 2011/11/18 19:57:02, agl wrote:
> Move it out of the class and do:
> 
> static const char kSHA256Descriptor[] = "NSS";
> 
> to save a relocation record.

Done.

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:50: if (!pickle->WriteInt(kSecureHashVersion))
On 2011/11/18 19:57:02, agl wrote:
> if (!pickle->WriteInt(kSecureHashVersion) ||
>     !pickle->WriteString(kDescriptor) ||
>     !pickle->WriteBytes(&ctx_, sizeof(ctx_))) {
>   return false;
> }

Done.

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:60: bool SecureHashSHA256NSS::Deserialize(Pickle*
pickle) {
On 2011/11/18 19:57:02, agl wrote:
> this interface (specifically the lack of a void* iterator param) precludes the
> serialized hash being part of a larger Pickle.

Fixed.

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_default.cc#n...
crypto/secure_hash_default.cc:79: char data_buffer[sizeof(ctx_)] = "";
On 2011/11/18 19:57:02, agl wrote:
> Pickle doesn't need a buffer to write it. It sets |data| to point into the
> underlying bytes.

Done.

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_openssl.cc
File crypto/secure_hash_openssl.cc (right):

http://codereview.chromium.org/8588057/diff/9/crypto/secure_hash_openssl.cc#n...
crypto/secure_hash_openssl.cc:20: static const int kSecureHashVersion = 1;
On 2011/11/18 19:57:02, agl wrote:
> all the same comments apply to this file.

Done.

[agl, 2011-11-18 21:26:16]

lgtm

[commit-bot: I haz the power, 2011-11-18 22:30:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8588057/8001

[commit-bot: I haz the power, 2011-11-18 23:43:52]

Try job failure for 8588057-8001 (retry) on linux_rel for steps "check_deps,
ui_tests".
It's a second try, previously, steps "check_deps, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-19 00:06:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8588057/8001

[commit-bot: I haz the power, 2011-11-19 01:51:29]

Try job failure for 8588057-8001 (retry) (retry) on linux_rel for step
"ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-19 15:24:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8588057/8001

[commit-bot: I haz the power, 2011-11-19 16:54:15]

Change committed as 110842