Add support to sha256 hash the downloaded file....

Add support to sha256 hash the downloaded file.

BUG=60822
TEST=base_file_unittest.cc


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

[lzheng, 2011-01-10 22:57:49]

This change will enable clients that have download_protection_enabled to
calculate sha256 hash for downloaded files.

I will hook up the looking up and report part in the next CL.

[Scott Hess - ex-Googler, 2011-01-10 23:12:07]

http://codereview.chromium.org/6023006/diff/43001/chrome/browser/download/bas...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6023006/diff/43001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:33: bool calculate_hash);
Is there any real advantage to making the hash calculation optional?  AFAICT, it
will kick in for all of the expensive cases, and not for the inexpensive cases,
so maybe it would be reasonable (and simpler) to always calculate it and only
use the results when necessary.

Alternately, WDYT about making the relevant methods virtual so that DownloadFile
can override them to calculate the digest and SaveFile can ignore?

http://codereview.chromium.org/6023006/diff/43001/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/6023006/diff/43001/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:85: 
Egregious whitespace change (looks like newline above |download_file| moved
below it.

[lzheng, 2011-01-11 01:06:44]

http://codereview.chromium.org/6023006/diff/43001/chrome/browser/download/bas...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6023006/diff/43001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:33: bool calculate_hash);
I made the "GetSha256Hash" virtual. 

I am still hesitate to let all downloads calculate the sha256 hash -- the hash
does not come free, if the hash is not needed, we should avoid it. Plus, the
"calculate_hash" flag does not make the code too complicated. Unless you have
other thoughts? 

On 2011/01/10 23:12:07, shess wrote:
> Is there any real advantage to making the hash calculation optional?  AFAICT,
it
> will kick in for all of the expensive cases, and not for the inexpensive
cases,
> so maybe it would be reasonable (and simpler) to always calculate it and only
> use the results when necessary.
> 
> Alternately, WDYT about making the relevant methods virtual so that
DownloadFile
> can override them to calculate the digest and SaveFile can ignore?

[Paweł Hajdan Jr., 2011-01-11 08:28:21]

Drive-by with minor download comments.

Also, can we avoid plumbing through the |calculate_hash| boolean, and instead
just always calculate it, for simplicity?

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
File chrome/browser/download/base_file.cc (right):

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.cc:76: SHA256_Update(sha_context_.get(),
Hmm, I wonder if we should somehow check result of file_stream_->Write below. If
that one fails, the calculated hash will get out of sync with real contents on
disk.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:22: typedef struct SHA256ContextStr
SHA256Context;
nit: Why do we need this typedef? It seems to add more confusion.

For example, something might #include base_file.h very indirectly, and rely on
SHA256Context typedef. Then, after some unrelated header reshuffling,
SHA256Context may no longer be visible.

Anyway, it's just weird to have two names for the same thing.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:62: // lower-case hex string. Returns false
if the digest is not calcuated.
nit: How about "Returns true on success" to avoid double negation?

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:78: static const size_t kSha256HashLen_ =
32;
nit: The underscore at the end looks weird, I think we don't use those for
constants.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:98: scoped_ptr<SHA256Context> sha_context_;
nit: Could you add a short comment?

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

Could you do a back-of-the-envelope calculation as to how much the hash
calculation costs?  All of your reviewers seem to be leaning towards the "always
do it" camp :-}.  

FWIW, we've got an outstanding bug where people are seeing 20% CPU utilization
during a download (the current suspect is animation).  While I want to fix that
bug, I'm not too worried (unless you tell me otherwise) that the hash
calculation would be a sizable percentage of that.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
File chrome/browser/download/base_file.cc (right):

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.cc:171:
StringToLowerASCII(base::HexEncode(sha256_hash_, sizeof(sha256_hash_)));
This means that the hash can be grabbed at any time during the download process,
and it'll be the hash to this point but not completed?  That seems like not what
you'd like; wouldn't you prefer GetSha256Hash to return false if the downloading
hasn't yet completed?

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:33: bool calculate_hash);
A tacky question, but I at least want to ask: What is the cost of always
calculating the hash?  The interface would be simpler and more constrained if
the file hash was just always available after the download had finished; you
wouldn't need to do all the messing around with constructors and creation
routines.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:62: // lower-case hex string. Returns false
if the digest is not calcuated.
At what point will the digest be calculated?  Is it tied to the completion of
the download?

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

On 2011/01/11 18:29:51, rdsmith wrote:
>
http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
> chrome/browser/download/base_file.cc:171:
> StringToLowerASCII(base::HexEncode(sha256_hash_, sizeof(sha256_hash_)));
> This means that the hash can be grabbed at any time during the download
process,
> and it'll be the hash to this point but not completed?  That seems like not
what
> you'd like; wouldn't you prefer GetSha256Hash to return false if the
downloading
> hasn't yet completed?

To be more explicit, unless there's some strong reason against in the expected
usage patterns, I'd rather have the SHA hash be defined as not available until
the download finishes.

[Scott Hess - ex-Googler, 2011-01-11 18:45:43]

On 2011/01/11 18:29:51, rdsmith wrote:
> Could you do a back-of-the-envelope calculation as to how much the hash
> calculation costs?  All of your reviewers seem to be leaning towards the
"always
> do it" camp :-}.  

Actually, I'm finding myself wondering if maybe the hash shouldn't be checked
for save-as-html.  Does the save-as do a check that the content is actually HTML
(as opposed to something which reports a mime-type of HTML)?  We will already
need the checked code path for save-as applied to an mp3 or other binary link.

[lzheng, 2011-01-12 02:11:54]

I believe I addressed everyone's concerns (see details below), except the "hash
or not hash" arguments.

As for computation cost, according to various source, sha256 is about 21 cycles
per byte. In terms of user-time, I did a comparison by "time wget
http://a_big_file" and "time sha256sum a_big_file", I noticed that the cpu time
spent caculating sha256sum is about the same as wget (but for the sha256, most
time are user cpu time, but wget is system cpu time). 

In summary, the cpu cost is not badly high (it is comparable with the amount of
cpu resource spending to wget a file), but it is not ignorable too.

That's said, the decision is not that obvious to make, if we want to calculate
sha256 for all cases, that will include the "save as" case Scott pointed out and
introduce some unnecessary cpu cycle.

So, I withdrew the previous approach but changed the Initialize() function in
BaseFile, which parse in a flag to indicate if we want hash to be calculated. It
is not as simple as calculate sha256 all the time, but should be better than
stick the flag in constructor.

What do you guys think now? I still try to avoid calculating sha256 all the
time, but if everyone is still in "always hash" camp, I could accept that.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
File chrome/browser/download/base_file.cc (right):

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.cc:76: SHA256_Update(sha_context_.get(),
On 2011/01/11 08:28:21, Paweł Hajdan Jr. wrote:
> Hmm, I wonder if we should somehow check result of file_stream_->Write below.
If
> that one fails, the calculated hash will get out of sync with real contents on
> disk.

Done.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:22: typedef struct SHA256ContextStr
SHA256Context;
I agree it looks confusing. Since the original SHA256Context is typedef'ed, to
avoid using the blapi.h header, that was the only way. 
I added the blapi.h header instead.

On 2011/01/11 08:28:21, Paweł Hajdan Jr. wrote:
> nit: Why do we need this typedef? It seems to add more confusion.
> 
> For example, something might #include base_file.h very indirectly, and rely on
> SHA256Context typedef. Then, after some unrelated header reshuffling,
> SHA256Context may no longer be visible.
> 
> Anyway, it's just weird to have two names for the same thing.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:33: bool calculate_hash);
Answered at the beginning of this email.

On 2011/01/11 18:29:51, rdsmith wrote:
> A tacky question, but I at least want to ask: What is the cost of always
> calculating the hash?  The interface would be simpler and more constrained if
> the file hash was just always available after the download had finished; you
> wouldn't need to do all the messing around with constructors and creation
> routines.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:62: // lower-case hex string. Returns false
if the digest is not calcuated.
On 2011/01/11 08:28:21, Paweł Hajdan Jr. wrote:
> nit: How about "Returns true on success" to avoid double negation?

Done.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:62: // lower-case hex string. Returns false
if the digest is not calcuated.
It should be. I added verification for the in_progress.
On 2011/01/11 18:29:51, rdsmith wrote:
> At what point will the digest be calculated?  Is it tied to the completion of
> the download?

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:78: static const size_t kSha256HashLen_ =
32;
On 2011/01/11 08:28:21, Paweł Hajdan Jr. wrote:
> nit: The underscore at the end looks weird, I think we don't use those for
> constants.

Done.

http://codereview.chromium.org/6023006/diff/55001/chrome/browser/download/bas...
chrome/browser/download/base_file.h:98: scoped_ptr<SHA256Context> sha_context_;
On 2011/01/11 08:28:21, Paweł Hajdan Jr. wrote:
> nit: Could you add a short comment?

Done.

[Randy Smith (Not in Mondays), 2011-01-12 14:47:27]

LGTM.

[Paweł Hajdan Jr., 2011-01-12 19:28:05]

LGTM, thanks

[Scott Hess - ex-Googler, 2011-01-13 22:58:08]

lgtm

http://codereview.chromium.org/6023006/diff/63003/chrome/browser/download/dow...
File chrome/browser/download/download_file.cc (left):

http://codereview.chromium.org/6023006/diff/63003/chrome/browser/download/dow...
chrome/browser/download/download_file.cc:28: 
Egregious whitespace change.  I agree with the change, but with no other changes
in this file, it's probably better to drop it.