base::MD5Clone() clones MD5Context.

[Superseded by https://codereview.chromium.org/211273009 ]

base::MD5Clone() clones MD5Context.

MD5Final() modifies the context, which prevents calculating an intermediate
digest while also continuing to build an uninterrupted context.

BUG=none
R=ajwong@chromium.org

[Scott Hess - ex-Googler, 2014-03-25 19:57:44]

Blame Sleevi.

My use case is that I'm using md5 as a checksum over a large file in safe
browsing.  The file has two parts, a header area and everything else.  The
header area can be read independently to make update requests, and sometimes the
data is corrupt, but reading the entire file to verify the checksum would
potentially waste I/O.  So I'd like to layer in a checksum after the header
info.

One would think that we'd have a better solution to this, like a fingerprint or
something, but AFAICT most such solutions calculate a result from a contiguous
in-memory data.  This data needs to be streamed incrementally.

Anyhow, my current code does the memcpy() locally, but depending on the internal
implementation from chrome/somewhere seems risky.

Thanks,
scott

[awong, 2014-03-25 20:00:41]

https://codereview.chromium.org/211503004/diff/1/base/md5.h
File base/md5.h (right):

https://codereview.chromium.org/211503004/diff/1/base/md5.h#newcode59
base/md5.h:59: // intermediate digest without affecting the original context.
This comment is a little bit confusing. Can you motivate with an example of how
to use and how it differs?

Also, what about adding a MD5IncrementalFinal() or something? That seems more
friendly than cloning, finalizing, and discarding cloned version...

[Ryan Sleevi, 2014-03-25 20:16:51]

On Mar 25, 2014 1:00 PM, <ajwong@chromium.org> wrote:
>
>
> https://codereview.chromium.org/211503004/diff/1/base/md5.h
> File base/md5.h (right):
>
> https://codereview.chromium.org/211503004/diff/1/base/md5.h#newcode59
> base/md5.h:59: // intermediate digest without affecting the original
> context.
> This comment is a little bit confusing. Can you motivate with an example
> of how to use and how it differs?
>
> Also, what about adding a MD5IncrementalFinal() or something? That seems
> more friendly than cloning, finalizing, and discarding cloned version...
>
> https://codereview.chromium.org/211503004/

The Clone approach is common in most crypto APIs - allowing the
implementation to duplicate any internal state necessary.

And IncrementalFinal would still clone under the hood.

Clone provides for more robust use cases (eg: PBKDF style derivations,
where you clone an initial state and then have parallel (different) updates
and finalization)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awong, 2014-03-25 20:18:33]

okay...moar doc plz?


On Tue, Mar 25, 2014 at 1:16 PM, Ryan Sleevi <rsleevi@chromium.org> wrote:

>
> On Mar 25, 2014 1:00 PM, <ajwong@chromium.org> wrote:
> >
> >
> > https://codereview.chromium.org/211503004/diff/1/base/md5.h
> > File base/md5.h (right):
> >
> > https://codereview.chromium.org/211503004/diff/1/base/md5.h#newcode59
> > base/md5.h:59: // intermediate digest without affecting the original
> > context.
> > This comment is a little bit confusing. Can you motivate with an example
> > of how to use and how it differs?
> >
> > Also, what about adding a MD5IncrementalFinal() or something? That seems
> > more friendly than cloning, finalizing, and discarding cloned version...
> >
> > https://codereview.chromium.org/211503004/
>
> The Clone approach is common in most crypto APIs - allowing the
> implementation to duplicate any internal state necessary.
>
> And IncrementalFinal would still clone under the hood.
>
> Clone provides for more robust use cases (eg: PBKDF style derivations,
> where you clone an initial state and then have parallel (different) updates
> and finalization)
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Scott Hess - ex-Googler, 2014-03-25 20:19:53]

On 2014/03/25 20:00:41, awong wrote:
> https://codereview.chromium.org/211503004/diff/1/base/md5.h
> File base/md5.h (right):
> 
> https://codereview.chromium.org/211503004/diff/1/base/md5.h#newcode59
> base/md5.h:59: // intermediate digest without affecting the original context.
> This comment is a little bit confusing. Can you motivate with an example of
how
> to use and how it differs?
> 
> Also, what about adding a MD5IncrementalFinal() or something? That seems more
> friendly than cloning, finalizing, and discarding cloned version...

Yay!  Because that's what I was originally thinking, except that nss had
MD5_Clone() so that also seemed sensible and more closely following pre-existing
reasons, whatever they are.

How about dueling CLs:
  https://codereview.chromium.org/211273009

[Scott Hess - ex-Googler, 2014-03-25 20:20:55]

Sorry for the write-after-write conflict.  I will also go upgrade this
one's documentation.


On Tue, Mar 25, 2014 at 1:19 PM, <shess@chromium.org> wrote:

> On 2014/03/25 20:00:41, awong wrote:
>
>> https://codereview.chromium.org/211503004/diff/1/base/md5.h
>> File base/md5.h (right):
>>
>
>  https://codereview.chromium.org/211503004/diff/1/base/md5.h#newcode59
>> base/md5.h:59: // intermediate digest without affecting the original
>> context.
>> This comment is a little bit confusing. Can you motivate with an example
>> of
>>
> how
>
>> to use and how it differs?
>>
>
>  Also, what about adding a MD5IncrementalFinal() or something? That seems
>> more
>> friendly than cloning, finalizing, and discarding cloned version...
>>
>
> Yay!  Because that's what I was originally thinking, except that nss had
> MD5_Clone() so that also seemed sensible and more closely following
> pre-existing
> reasons, whatever they are.
>
> How about dueling CLs:
>   https://codereview.chromium.org/211273009
>
>
> https://codereview.chromium.org/211503004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Scott Hess - ex-Googler, 2014-03-25 20:36:39]

Updated the comment.  I think it's still atrocious.

I don't fully agree with the generality argument, given that this is md5
(already established: only dim people use md5) and given that this many years in
I'm the first person who apparently decided to even clone for calculating an
intermediate digest.  But, by the same token, I'll probably be the last person
to modify this file other than general code-health refactors, so it probably
doesn't matter.

On 2014/03/25 20:18:33, awong wrote:
> okay...moar doc plz?
> 
> 
> On Tue, Mar 25, 2014 at 1:16 PM, Ryan Sleevi <mailto:rsleevi@chromium.org>
wrote:
> 
> >
> > On Mar 25, 2014 1:00 PM, <mailto:ajwong@chromium.org> wrote:
> > >
> > >
> > > https://codereview.chromium.org/211503004/diff/1/base/md5.h
> > > File base/md5.h (right):
> > >
> > > https://codereview.chromium.org/211503004/diff/1/base/md5.h#newcode59
> > > base/md5.h:59: // intermediate digest without affecting the original
> > > context.
> > > This comment is a little bit confusing. Can you motivate with an example
> > > of how to use and how it differs?
> > >
> > > Also, what about adding a MD5IncrementalFinal() or something? That seems
> > > more friendly than cloning, finalizing, and discarding cloned version...
> > >
> > > https://codereview.chromium.org/211503004/
> >
> > The Clone approach is common in most crypto APIs - allowing the
> > implementation to duplicate any internal state necessary.
> >
> > And IncrementalFinal would still clone under the hood.
> >
> > Clone provides for more robust use cases (eg: PBKDF style derivations,
> > where you clone an initial state and then have parallel (different) updates
> > and finalization)
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[awong, 2014-03-25 20:40:48]

@sleevi: how often do we actually expect people in Chrome to clone contexts
except in this incremental generation case?

If it's near 0...why not just do the simple API? On further though,
convention doesn't seem to apply.

-Albert


On Tue, Mar 25, 2014 at 1:36 PM, <shess@chromium.org> wrote:

> Updated the comment.  I think it's still atrocious.
>
> I don't fully agree with the generality argument, given that this is md5
> (already established: only dim people use md5) and given that this many
> years in
> I'm the first person who apparently decided to even clone for calculating
> an
> intermediate digest.  But, by the same token, I'll probably be the last
> person
> to modify this file other than general code-health refactors, so it
> probably
> doesn't matter.
>
> On 2014/03/25 20:18:33, awong wrote:
>
>> okay...moar doc plz?
>>
>
>
>  On Tue, Mar 25, 2014 at 1:16 PM, Ryan Sleevi <mailto:rsleevi@chromium.org
>> >
>>
> wrote:
>
>  >
>> > On Mar 25, 2014 1:00 PM, <mailto:ajwong@chromium.org> wrote:
>> > >
>> > >
>> > > https://codereview.chromium.org/211503004/diff/1/base/md5.h
>> > > File base/md5.h (right):
>> > >
>> > > https://codereview.chromium.org/211503004/diff/1/base/md5.h#newcode59
>> > > base/md5.h:59: // intermediate digest without affecting the original
>> > > context.
>> > > This comment is a little bit confusing. Can you motivate with an
>> example
>> > > of how to use and how it differs?
>> > >
>> > > Also, what about adding a MD5IncrementalFinal() or something? That
>> seems
>> > > more friendly than cloning, finalizing, and discarding cloned
>> version...
>> > >
>> > > https://codereview.chromium.org/211503004/
>> >
>> > The Clone approach is common in most crypto APIs - allowing the
>> > implementation to duplicate any internal state necessary.
>> >
>> > And IncrementalFinal would still clone under the hood.
>> >
>> > Clone provides for more robust use cases (eg: PBKDF style derivations,
>> > where you clone an initial state and then have parallel (different)
>> updates
>> > and finalization)
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/211503004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2014-03-25 21:09:56]

On 2014/03/25 20:40:48, awong wrote:
> @sleevi: how often do we actually expect people in Chrome to clone contexts
> except in this incremental generation case?
> 
> If it's near 0...why not just do the simple API? On further though,
> convention doesn't seem to apply.
> 
> -Albert

Main argument is principle of least surprise. We do use clone of hashes in other
places (eg: PBKDF2, as implemented in
https://code.google.com/p/chromium/codesearch#chromium/src/crypto/symmetric_k...
)

You're entirely correct that this is our code, so it doesn't really matter what
API we chose, we can always refactor it later. I lean on MD5Clone for
consistency, but it's not a strong argument. It just keeps the symmetry with the
Init/Update/Final approach.

[awong, 2014-03-25 21:37:00]

It sounds rare enough that we should go with the simple one first.  Heck,
even if you add clone, it sound like a less error-prone wrapper for those
who aren't quite as fluent in crypto APIs.

-Albert


On Tue, Mar 25, 2014 at 2:09 PM, <rsleevi@chromium.org> wrote:

> On 2014/03/25 20:40:48, awong wrote:
>
>> @sleevi: how often do we actually expect people in Chrome to clone
>> contexts
>> except in this incremental generation case?
>>
>
>  If it's near 0...why not just do the simple API? On further though,
>> convention doesn't seem to apply.
>>
>
>  -Albert
>>
>
> Main argument is principle of least surprise. We do use clone of hashes in
> other
> places (eg: PBKDF2, as implemented in
> https://code.google.com/p/chromium/codesearch#chromium/
> src/crypto/symmetric_key_win.cc&l=283
> )
>
> You're entirely correct that this is our code, so it doesn't really matter
> what
> API we chose, we can always refactor it later. I lean on MD5Clone for
> consistency, but it's not a strong argument. It just keeps the symmetry
> with the
> Init/Update/Final approach.
>
> https://codereview.chromium.org/211503004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2014-03-26 17:45:13]

On 2014/03/25 21:37:00, awong wrote:
> It sounds rare enough that we should go with the simple one first.  Heck,
> even if you add clone, it sound like a less error-prone wrapper for those
> who aren't quite as fluent in crypto APIs.
> 
> -Albert

Not entirely sure I get what you mean by "less error prone" (just that you
finalize/delete what you copy?)

Anyways, I suspect it's a debate about high-level API composed of two ops versus
a low-level API that exposes both ops, and for this use case, either is fine.