code review 65690043: crypto/rc4: test the portable version too

crypto/rc4: test the portable version too

Prevent bitrot. (similar to the previous sha1 and md5 CLs)

Fixes Issue 6642

[bradfitz, 2014-02-18 22:43:25]

Hello agl@chromium.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2014-02-18 22:45:20]

Don't forget the Fixes issue line

> On 19 Feb 2014, at 9:43, bradfitz@golang.org wrote:
> 
> Reviewers: agl,
> 
> Message:
> Hello agl@chromium.org (cc: golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/
> 
> 
> Description:
> crypto/rc4: test the portable version too
> 
> Prevent bitrot. (similar to the previous sha1 and md5 CLs)
> 
> Please review this at https://codereview.appspot.com/65690043/
> 
> Affected files (+33, -12 lines):
>  M src/pkg/crypto/rc4/rc4.go
>  M src/pkg/crypto/rc4/rc4_ref.go
>  M src/pkg/crypto/rc4/rc4_test.go
> 
> 
> Index: src/pkg/crypto/rc4/rc4.go
> ===================================================================
> --- a/src/pkg/crypto/rc4/rc4.go
> +++ b/src/pkg/crypto/rc4/rc4.go
> @@ -50,3 +50,20 @@
>    }
>    c.i, c.j = 0, 0
> }
> +
> +// xorKeyStreamGeneric sets dst to the result of XORing src with the
> +// key stream.  Dst and src may be the same slice but otherwise should
> +// not overlap.
> +//
> +// This is the pure Go version. rc4_{amd64,386,arm}* contain assembly
> +// implementations. This is here for tests and to prevent bitrot.
> +func (c *Cipher) xorKeyStreamGeneric(dst, src []byte) {
> +    i, j := c.i, c.j
> +    for k, v := range src {
> +        i += 1
> +        j += uint8(c.s[i])
> +        c.s[i], c.s[j] = c.s[j], c.s[i]
> +        dst[k] = v ^ uint8(c.s[uint8(c.s[i]+c.s[j])])
> +    }
> +    c.i, c.j = i, j
> +}
> Index: src/pkg/crypto/rc4/rc4_ref.go
> ===================================================================
> --- a/src/pkg/crypto/rc4/rc4_ref.go
> +++ b/src/pkg/crypto/rc4/rc4_ref.go
> @@ -9,12 +9,5 @@
> // XORKeyStream sets dst to the result of XORing src with the key stream.
> // Dst and src may be the same slice but otherwise should not overlap.
> func (c *Cipher) XORKeyStream(dst, src []byte) {
> -    i, j := c.i, c.j
> -    for k, v := range src {
> -        i += 1
> -        j += uint8(c.s[i])
> -        c.s[i], c.s[j] = c.s[j], c.s[i]
> -        dst[k] = v ^ uint8(c.s[uint8(c.s[i]+c.s[j])])
> -    }
> -    c.i, c.j = i, j
> +    return c.xorKeyStreamGeneric(dst, src)
> }
> Index: src/pkg/crypto/rc4/rc4_test.go
> ===================================================================
> --- a/src/pkg/crypto/rc4/rc4_test.go
> +++ b/src/pkg/crypto/rc4/rc4_test.go
> @@ -117,19 +117,30 @@
> }
> 
> func TestBlock(t *testing.T) {
> +    testBlock(t, (*Cipher).XORKeyStream)
> +}
> +
> +// Test the pure Go version.
> +// Because we have assembly for amd64, 386, and arm, this prevents
> +// bitrot of the reference implementations.
> +func TestBlockGeneric(t *testing.T) {
> +    testBlock(t, (*Cipher).xorKeyStreamGeneric)
> +}
> +
> +func testBlock(t *testing.T, xor func(c *Cipher, dst, src []byte)) {
>    c1a, _ := NewCipher(golden[0].key)
>    c1b, _ := NewCipher(golden[1].key)
>    data1 := make([]byte, 1<<20)
>    for i := range data1 {
> -        c1a.XORKeyStream(data1[i:i+1], data1[i:i+1])
> -        c1b.XORKeyStream(data1[i:i+1], data1[i:i+1])
> +        xor(c1a, data1[i:i+1], data1[i:i+1])
> +        xor(c1b, data1[i:i+1], data1[i:i+1])
>    }
> 
>    c2a, _ := NewCipher(golden[0].key)
>    c2b, _ := NewCipher(golden[1].key)
>    data2 := make([]byte, 1<<20)
> -    c2a.XORKeyStream(data2, data2)
> -    c2b.XORKeyStream(data2, data2)
> +    xor(c2a, data2, data2)
> +    xor(c2b, data2, data2)
> 
>    if !bytes.Equal(data1, data2) {
>        t.Fatalf("bad block")
> 
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[bradfitz, 2014-02-18 22:46:43]

Hello agl@chromium.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-02-18 22:47:05]

I didn't even know there was a bug open.  :-)



On Tue, Feb 18, 2014 at 2:45 PM, Dave Cheney <dave@cheney.net> wrote:

> Don't forget the Fixes issue line
>
> > On 19 Feb 2014, at 9:43, bradfitz@golang.org wrote:
> >
> > Reviewers: agl,
> >
> > Message:
> > Hello agl@chromium.org (cc: golang-codereviews@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://go.googlecode.com/hg/
> >
> >
> > Description:
> > crypto/rc4: test the portable version too
> >
> > Prevent bitrot. (similar to the previous sha1 and md5 CLs)
> >
> > Please review this at https://codereview.appspot.com/65690043/
> >
> > Affected files (+33, -12 lines):
> >  M src/pkg/crypto/rc4/rc4.go
> >  M src/pkg/crypto/rc4/rc4_ref.go
> >  M src/pkg/crypto/rc4/rc4_test.go
> >
> >
> > Index: src/pkg/crypto/rc4/rc4.go
> > ===================================================================
> > --- a/src/pkg/crypto/rc4/rc4.go
> > +++ b/src/pkg/crypto/rc4/rc4.go
> > @@ -50,3 +50,20 @@
> >    }
> >    c.i, c.j = 0, 0
> > }
> > +
> > +// xorKeyStreamGeneric sets dst to the result of XORing src with the
> > +// key stream.  Dst and src may be the same slice but otherwise should
> > +// not overlap.
> > +//
> > +// This is the pure Go version. rc4_{amd64,386,arm}* contain assembly
> > +// implementations. This is here for tests and to prevent bitrot.
> > +func (c *Cipher) xorKeyStreamGeneric(dst, src []byte) {
> > +    i, j := c.i, c.j
> > +    for k, v := range src {
> > +        i += 1
> > +        j += uint8(c.s[i])
> > +        c.s[i], c.s[j] = c.s[j], c.s[i]
> > +        dst[k] = v ^ uint8(c.s[uint8(c.s[i]+c.s[j])])
> > +    }
> > +    c.i, c.j = i, j
> > +}
> > Index: src/pkg/crypto/rc4/rc4_ref.go
> > ===================================================================
> > --- a/src/pkg/crypto/rc4/rc4_ref.go
> > +++ b/src/pkg/crypto/rc4/rc4_ref.go
> > @@ -9,12 +9,5 @@
> > // XORKeyStream sets dst to the result of XORing src with the key stream.
> > // Dst and src may be the same slice but otherwise should not overlap.
> > func (c *Cipher) XORKeyStream(dst, src []byte) {
> > -    i, j := c.i, c.j
> > -    for k, v := range src {
> > -        i += 1
> > -        j += uint8(c.s[i])
> > -        c.s[i], c.s[j] = c.s[j], c.s[i]
> > -        dst[k] = v ^ uint8(c.s[uint8(c.s[i]+c.s[j])])
> > -    }
> > -    c.i, c.j = i, j
> > +    return c.xorKeyStreamGeneric(dst, src)
> > }
> > Index: src/pkg/crypto/rc4/rc4_test.go
> > ===================================================================
> > --- a/src/pkg/crypto/rc4/rc4_test.go
> > +++ b/src/pkg/crypto/rc4/rc4_test.go
> > @@ -117,19 +117,30 @@
> > }
> >
> > func TestBlock(t *testing.T) {
> > +    testBlock(t, (*Cipher).XORKeyStream)
> > +}
> > +
> > +// Test the pure Go version.
> > +// Because we have assembly for amd64, 386, and arm, this prevents
> > +// bitrot of the reference implementations.
> > +func TestBlockGeneric(t *testing.T) {
> > +    testBlock(t, (*Cipher).xorKeyStreamGeneric)
> > +}
> > +
> > +func testBlock(t *testing.T, xor func(c *Cipher, dst, src []byte)) {
> >    c1a, _ := NewCipher(golden[0].key)
> >    c1b, _ := NewCipher(golden[1].key)
> >    data1 := make([]byte, 1<<20)
> >    for i := range data1 {
> > -        c1a.XORKeyStream(data1[i:i+1], data1[i:i+1])
> > -        c1b.XORKeyStream(data1[i:i+1], data1[i:i+1])
> > +        xor(c1a, data1[i:i+1], data1[i:i+1])
> > +        xor(c1b, data1[i:i+1], data1[i:i+1])
> >    }
> >
> >    c2a, _ := NewCipher(golden[0].key)
> >    c2b, _ := NewCipher(golden[1].key)
> >    data2 := make([]byte, 1<<20)
> > -    c2a.XORKeyStream(data2, data2)
> > -    c2b.XORKeyStream(data2, data2)
> > +    xor(c2a, data2, data2)
> > +    xor(c2b, data2, data2)
> >
> >    if !bytes.Equal(data1, data2) {
> >        t.Fatalf("bad block")
> >
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
>

[agl1, 2014-02-18 23:13:29]

LGTM, thanks!

[bradfitz, 2014-02-18 23:16:09]

*** Submitted as https://code.google.com/p/go/source/detail?r=788582b475b5 ***

crypto/rc4: test the portable version too

Prevent bitrot. (similar to the previous sha1 and md5 CLs)

Fixes Issue 6642

LGTM=agl
R=agl, dave
CC=golang-codereviews
https://codereview.appspot.com/65690043

[gobot, 2014-02-18 23:21:32]

This CL appears to have broken the freebsd-amd64 builder.