New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/salsa20/salsa: confusing docs #21279

Closed
bradfitz opened this Issue Aug 2, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@bradfitz
Member

bradfitz commented Aug 2, 2017

The docs for XORKeyStream say:

// XORKeyStream crypts bytes from in to out using the given key and counters.
// In and out may be the same slice but otherwise should not overlap. Counter
// contains the raw salsa20 counter bytes (both nonce and block counter).
func XORKeyStream(out, in []byte, counter *[16]byte, key *[32]byte) {

What does this even mean: ?

may be the same slice but otherwise should not overlap

If they're the same slice (same 3-tuple ptr/len/cap) then they by definition overlap.

Fix this and any copy/pastes of this wording we find.

@gopherbot gopherbot added this to the Unreleased milestone Aug 2, 2017

@bradfitz bradfitz added the help wanted label Aug 2, 2017

@ALTree

This comment has been minimized.

Show comment
Hide comment
@ALTree

ALTree Aug 2, 2017

Member

I think the key word is otherwise

In and out may be the same slice but otherwise should not overlap.

Same slice is ok, and it that case they'll overlap (obviously), but otherwise they shouldn't overlap.

Member

ALTree commented Aug 2, 2017

I think the key word is otherwise

In and out may be the same slice but otherwise should not overlap.

Same slice is ok, and it that case they'll overlap (obviously), but otherwise they shouldn't overlap.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Aug 2, 2017

Member

Huh? I don't think that helps.

Member

bradfitz commented Aug 2, 2017

Huh? I don't think that helps.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 3, 2017

Member

How about

in and out must overlap entirely or not at all.

Member

FiloSottile commented Aug 3, 2017

How about

in and out must overlap entirely or not at all.

@kreichgauer

This comment has been minimized.

Show comment
Hide comment
@kreichgauer

kreichgauer Aug 3, 2017

Contributor

if in and out overlap, they must be identical

is how it's usually worded in BoringSSL

Contributor

kreichgauer commented Aug 3, 2017

if in and out overlap, they must be identical

is how it's usually worded in BoringSSL

@agl

This comment has been minimized.

Show comment
Hide comment
@agl

agl Aug 3, 2017

Contributor

The intent is to describe that there are only two valid situations:

  1. in and out are the same slice.
  2. in and out do not alias at all.

Other configurations (i.e. in and out partially overlap) may not produce the correct result. As Martin notes, we often use a slightly different wording in BoringSSL. I fear that I'm too close to the trees to have a valid judgement about what would be clearer so someone else is free to pick something and change the wording.

Contributor

agl commented Aug 3, 2017

The intent is to describe that there are only two valid situations:

  1. in and out are the same slice.
  2. in and out do not alias at all.

Other configurations (i.e. in and out partially overlap) may not produce the correct result. As Martin notes, we often use a slightly different wording in BoringSSL. I fear that I'm too close to the trees to have a valid judgement about what would be clearer so someone else is free to pick something and change the wording.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 3, 2017

Member
Member

FiloSottile commented Aug 3, 2017

@crvv

This comment has been minimized.

Show comment
Hide comment
@crvv

crvv Aug 9, 2017

Contributor

Maybe
out[0:1] and in[1:] must not overlap.

I think this is accurate but a little obscure.

Contributor

crvv commented Aug 9, 2017

Maybe
out[0:1] and in[1:] must not overlap.

I think this is accurate but a little obscure.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 28, 2017

Member

We should also put that text in the cipher.Stream.XORKeyStream docs.

The current text probably means this, but leaves too much open to interpretation, and implementations are already more strict anyway:

Dst and src may point to the same memory.

For the sake of breaking the bikeshed, I'm going to CL "must overlap entirely or not at all" tomorrow unless anyone objects.

Member

FiloSottile commented Aug 28, 2017

We should also put that text in the cipher.Stream.XORKeyStream docs.

The current text probably means this, but leaves too much open to interpretation, and implementations are already more strict anyway:

Dst and src may point to the same memory.

For the sake of breaking the bikeshed, I'm going to CL "must overlap entirely or not at all" tomorrow unless anyone objects.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 3, 2017

Change https://golang.org/cl/61133 mentions this issue: all: make overlap rules wording consistent

gopherbot commented Sep 3, 2017

Change https://golang.org/cl/61133 mentions this issue: all: make overlap rules wording consistent

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 3, 2017

Change https://golang.org/cl/61132 mentions this issue: crypto/cipher, crypto/rc4: make overlap rules wording consistent

gopherbot commented Sep 3, 2017

Change https://golang.org/cl/61132 mentions this issue: crypto/cipher, crypto/rc4: make overlap rules wording consistent

gopherbot pushed a commit to golang/crypto that referenced this issue Sep 11, 2017

all: make overlap rules wording consistent
Updates golang/go#21279

Change-Id: I686835c644f52e3d5ea2b7e6431ef096d188c19d
Reviewed-on: https://go-review.googlesource.com/61133
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@gopherbot gopherbot closed this in 6fac139 Oct 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment