Add base::SHA1HashBytes

Add base::SHA1HashBytes.

We need this for LevelDB.

BUG=64078
TEST=base_unittest --gtest_filter=SHA1Test

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

[wtc, 2011-03-10 17:49:33]

hans: I maintain these two files.  I can review this CL for
you.

The two files were copied from the NSS crypto library to
support the SHA-256 hash algorithm.  I pruned the files
so that they contain only what SHA-256 needs.

So it would be wrong to add the SHA-1 types and declarations
back to the copies of these two files in base/third_party/nss.

Note that we already have SHA-1 in base/crypto.

[jorlow, 2011-03-10 17:51:01]

LGTM...wait for wtc tho

[hans, 2011-03-10 18:02:31]

On 2011/03/10 17:49:33, wtc wrote:
> hans: I maintain these two files.  I can review this CL for
> you.
> 
> The two files were copied from the NSS crypto library to
> support the SHA-256 hash algorithm.  I pruned the files
> so that they contain only what SHA-256 needs.
> 
> So it would be wrong to add the SHA-1 types and declarations
> back to the copies of these two files in base/third_party/nss.
> 
> Note that we already have SHA-1 in base/crypto.

Ok, we'll add a new SHA1Hash that takes raw bytes to that file. Thanks.

[hans, 2011-03-11 15:44:56]

On 2011/03/10 18:02:31, hans wrote:
> On 2011/03/10 17:49:33, wtc wrote:
> > hans: I maintain these two files.  I can review this CL for
> > you.
> > 
> > The two files were copied from the NSS crypto library to
> > support the SHA-256 hash algorithm.  I pruned the files
> > so that they contain only what SHA-256 needs.
> > 
> > So it would be wrong to add the SHA-1 types and declarations
> > back to the copies of these two files in base/third_party/nss.
> > 
> > Note that we already have SHA-1 in base/crypto.
> 
> Ok, we'll add a new SHA1Hash that takes raw bytes to that file. Thanks.

Right, new approach: I'm adding base::SHA1HashBytes. This makes it much easier
for us since LevelDB doesn't have to worry about depending on the right crypto
library.

I first figured I should update sha1_win.cc, but then realized that since
r50900, that file isn't used. (And it looks slightly broken.)

Please take a look.

[jorlow, 2011-03-11 17:29:34]

LGTM

[wtc, 2011-03-11 18:46:15]

LGTM.  Please make the changes I suggested below before
checking this in.  Feel free to ignore the comments marked
with "Nit".

IMPORTANT: Chromium uses std::string to hold binary data.
(This is confusing to a C programmer like me.)  I just wanted
to make sure SHA1HashString is not suitable for you, for
example, you want to avoid the overhead of copying the
input or output to/from a std::string.

http://codereview.chromium.org/6661025/diff/9001/base/sha1.h
File base/sha1.h (right):

http://codereview.chromium.org/6661025/diff/9001/base/sha1.h#newcode26
base/sha1.h:26: unsigned char* hash_array);
Nit: |hash_array| can be shortened to |hash|.

Please document that |hash_array| must be SHA1_LENGTH bytes
long.

Alternatively, force the caller to specify the length of
|hash_array|:

void SHA1HashBytes(const unsigned char* data, size_t data_len,
                   unsigned char* hash, size_t hash_len);

http://codereview.chromium.org/6661025/diff/9001/base/sha1_portable.cc
File base/sha1_portable.cc (right):

http://codereview.chromium.org/6661025/diff/9001/base/sha1_portable.cc#newcode8
base/sha1_portable.cc:8: #include <cstring>
1. Nit: I recommend including the C header <string.h>
instead of <cstring> for consistency with existing usage
in the Chromium code base.  See line 25, for example.  (I
know it's inside a comment.)

2. The headers should be listed in this order:

#include "base/sha1.h"

#include <string.h> // or <cstring>

#include "base/basictypes.h"

See the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/6661025/diff/9001/base/sha1_portable.cc#newcod...
base/sha1_portable.cc:198: std::string SHA1HashString(const std::string& str) {
Nit: ideally this function should be reimplemented using
SHA1HashBytes.

[hans, 2011-03-13 19:48:16]

Thanks for the review.

> IMPORTANT: Chromium uses std::string to hold binary data.
> (This is confusing to a C programmer like me.)
Me too :)

> I just wanted
> to make sure SHA1HashString is not suitable for you, for
> example, you want to avoid the overhead of copying the
> input or output to/from a std::string.
Yes, we plan to call this a lot, so want to avoid the extra copying.

I might also look into crbug.com/47218 if this can affect our performance.

http://codereview.chromium.org/6661025/diff/9001/base/sha1.h
File base/sha1.h (right):

http://codereview.chromium.org/6661025/diff/9001/base/sha1.h#newcode26
base/sha1.h:26: unsigned char* hash_array);
On 2011/03/11 18:46:15, wtc wrote:
> Nit: |hash_array| can be shortened to |hash|.
done

> 
> Please document that |hash_array| must be SHA1_LENGTH bytes
> long.
done

http://codereview.chromium.org/6661025/diff/9001/base/sha1_portable.cc
File base/sha1_portable.cc (right):

http://codereview.chromium.org/6661025/diff/9001/base/sha1_portable.cc#newcode8
base/sha1_portable.cc:8: #include <cstring>
On 2011/03/11 18:46:15, wtc wrote:
> 1. Nit: I recommend including the C header <string.h>
> instead of <cstring> for consistency with existing usage
> in the Chromium code base.  See line 25, for example.  (I
> know it's inside a comment.)
done

> 
> 2. The headers should be listed in this order:
> 
> #include "base/sha1.h"
> 
> #include <string.h> // or <cstring>
> 
> #include "base/basictypes.h"
> 
> See the Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

done

http://codereview.chromium.org/6661025/diff/9001/base/sha1_portable.cc#newcod...
base/sha1_portable.cc:198: std::string SHA1HashString(const std::string& str) {
On 2011/03/11 18:46:15, wtc wrote:
> Nit: ideally this function should be reimplemented using
> SHA1HashBytes.

Done.