code review 7679043: encoding/base64: Add a reader that discard \n and \r

encoding/base64: Add a reader that discard \n and \r

The decoder gets confused by \n and \r, so filter these out
using a small Reader that discards these.

The same issue applies to the encoding/base32 package.
Fixes issue 4779

[Miek Gieben, 2013-03-10 07:45:14]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-03-10 15:42:07]

The first line of your commit description should summarize what it fixes,
not how it fixes it.  A reader is an implementation detail, and this almost
makes it sound like you're adding a new public API (add a reader!, until I
see the actual patch.  Maybe just "ignore newlines when decoding".

Also, regarding the TODO--- why not just fix it now, as long as you're
here?  If it's something that needs fixing.  I looked at
https://tools.ietf.org/html/rfc4648#section-3.1 which is referenced from
the issue, but I'm not sure that's what the TODO is referencing.  What is
special about \r\n together?


On Sat, Mar 9, 2013 at 11:45 PM, <remigius.gieben@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> encoding/base64: Add a reader that discard \n and \r
>
> The decoder gets confused by \n and \r, so filter these out
> using a small Reader that discards these.
>
> The same issue applies to the encoding/base32 package.
> Fixes issue 4779
>
> Please review this at
https://codereview.appspot.**com/7679043/<https://codereview.appspot.com/7679...
>
> Affected files:
>   M src/pkg/encoding/base32/**base32.go
>   M src/pkg/encoding/base32/**base32_test.go
>   M src/pkg/encoding/base64/**base64.go
>   M src/pkg/encoding/base64/**base64_test.go
>
>
> Index: src/pkg/encoding/base32/**base32.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/base32/**base32.go
> +++ b/src/pkg/encoding/base32/**base32.go
> @@ -220,6 +220,28 @@
>   * Decoder
>   */
>
> +// Reader thats discards \n and \r from its input
> +type discardReader struct {
> +       io.Reader
> +}
> +
> +func (d *discardReader) Read(p []byte) (n int, err error) {
> +       n, err = d.Reader.Read(p)
> +       // TODO: could check for adjacent \n and \r's
> +       for i := 0; i < n; i++ {
> +               if p[i] == '\n' || p[i] == '\r' {
> +                       if i+1 == n {
> +                               n--
> +                       } else {
> +                               p = append(p[:i], p[i+1:]...)
> +                               n--
> +                               i--
> +                       }
> +               }
> +       }
> +       return
> +}
> +
>  type CorruptInputError int64
>
>  func (e CorruptInputError) Error() string {
> @@ -379,7 +401,7 @@
>
>  // NewDecoder constructs a new base32 stream decoder.
>  func NewDecoder(enc *Encoding, r io.Reader) io.Reader {
> -       return &decoder{enc: enc, r: r}
> +       return &decoder{enc: enc, r: &discardReader{r}}
>  }
>
>  // DecodedLen returns the maximum length in bytes of the decoded data
> Index: src/pkg/encoding/base32/**base32_test.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/base32/**base32_test.go
> +++ b/src/pkg/encoding/base32/**base32_test.go
> @@ -15,6 +15,10 @@
>         decoded, encoded string
>  }
>
> +type testtriplet struct {
> +       decoded, encodedNewline, encoded string
> +}
> +
>  var pairs = []testpair{
>         // RFC 4648 examples
>         {"", ""},
> @@ -36,6 +40,14 @@
>         {"sure.", "ON2XEZJO"},
>  }
>
> +var triplets = []testtriplet{
> +       {"", "\n", ""},
> +       {"f", "M\r\nY======", "MY======"},
> +       {"fo", "MZXQ====\n\r", "MZXQ===="},
> +       {"foo", "MZXW\r6=\n==", "MZXW6==="},
> +       {"foob", "MZXW6Y\rQ=", "MZXW6YrQ="},
> +}
> +
>  var bigtest = testpair{
>         "Twas brillig, and the slithy toves",
>         "**KR3WC4ZAMJZGS3DMNFTSYIDBNZSCA5**DIMUQHG3DJORUHSIDUN53GK4Y=",
> @@ -136,6 +148,16 @@
>         }
>  }
>
> +// Issue is about base64, but is needed for base32 too.
> +func TestDecoderIssue4779(t *testing.T) {
> +       for _, p := range triplets {
> +               decoder := NewDecoder(StdEncoding, bytes.NewBufferString(p.
> **encodedNewline))
> +               dbuf, err := ioutil.ReadAll(decoder)
> +               testEqual(t, "Decoding of %q = error %v, want %v",
> p.encodedNewline, err, error(nil))
> +               testEqual(t, "Decoding of %q = %q, want %q",
> p.encodedNewline, string(dbuf), p.decoded)
> +       }
> +}
> +
>  func TestDecodeCorrupt(t *testing.T) {
>         type corrupt struct {
>                 e string
> Index: src/pkg/encoding/base64/**base64.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/base64/**base64.go
> +++ b/src/pkg/encoding/base64/**base64.go
> @@ -200,6 +200,28 @@
>   * Decoder
>   */
>
> +// Reader thats discards \n and \r from its input
> +type discardReader struct {
> +       io.Reader
> +}
> +
> +func (d *discardReader) Read(p []byte) (n int, err error) {
> +       n, err = d.Reader.Read(p)
> +       // TODO: could check for adjacent \n and \r's
> +       for i := 0; i < n; i++ {
> +               if p[i] == '\n' || p[i] == '\r' {
> +                       if i+1 == n {
> +                               n--
> +                       } else {
> +                               p = append(p[:i], p[i+1:]...)
> +                               n--
> +                               i--
> +                       }
> +               }
> +       }
> +       return
> +}
> +
>  type CorruptInputError int64
>
>  func (e CorruptInputError) Error() string {
> @@ -345,7 +367,7 @@
>
>  // NewDecoder constructs a new base64 stream decoder.
>  func NewDecoder(enc *Encoding, r io.Reader) io.Reader {
> -       return &decoder{enc: enc, r: r}
> +       return &decoder{enc: enc, r: &discardReader{r}}
>  }
>
>  // DecodedLen returns the maximum length in bytes of the decoded data
> Index: src/pkg/encoding/base64/**base64_test.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/base64/**base64_test.go
> +++ b/src/pkg/encoding/base64/**base64_test.go
> @@ -17,6 +17,10 @@
>         decoded, encoded string
>  }
>
> +type testtriplet struct {
> +       decoded, encodedNewline, encoded string
> +}
> +
>  var pairs = []testpair{
>         // RFC 3548 examples
>         {"\x14\xfb\x9c\x03\xd9\x7e", "FPucA9l+"},
> @@ -43,6 +47,19 @@
>         {"sure.", "c3VyZS4="},
>  }
>
> +var triplets = []testtriplet{
> +       {"\x14\xfb\x9c\x03\xd9\x7e", "\nFPucA9l+", "FPucA9l+"},
> +       {"\x14\xfb\x9c\x03\xd9", "F\nPucA9k=", "FPucA9k="},
> +       {"\x14\xfb\x9c\x03", "FPucAw=\n=", "FPucAw=="},
> +       {"\x14\xfb\x9c\x03", "FPucAw=\n=\n", "FPucAw=="},
> +
> +       {"", "\n", ""},
> +       {"f", "Z\r\ng==", "Zg=="},
> +       {"fo", "Zm8=\n\r", "Zm8=r"},
> +       {"foo", "Zm9\rv", "Zm9v"},
> +       {"foob", "Zm9vYg=\r=", "Zm9vYg=="},
> +}
> +
>  var bigtest = testpair{
>         "Twas brillig, and the slithy toves",
>         "**VHdhcyBicmlsbGlnLCBhbmQgdGhlIH**NsaXRoeSB0b3Zlcw==",
> @@ -141,6 +158,15 @@
>         }
>  }
>
> +func TestDecoderIssue4779(t *testing.T) {
> +       for _, p := range triplets {
> +               decoder := NewDecoder(StdEncoding, bytes.NewBufferString(p.
> **encodedNewline))
> +               dbuf, err := ioutil.ReadAll(decoder)
> +               testEqual(t, "Decoding of %q = error %v, want %v",
> p.encodedNewline, err, error(nil))
> +               testEqual(t, "Decoding of %q = %q, want %q",
> p.encodedNewline, string(dbuf), p.decoded)
> +       }
> +}
> +
>  func TestDecodeCorrupt(t *testing.T) {
>         type corrupt struct {
>                 e string
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[Miek Gieben, 2013-03-11 15:25:41]

Ok, I'll change the first line in the commit message.

The TODO is about looking for multiple \n and \r next to each other, to minimize
the mem-moves and make it faster. But I hope I can get away with just removing
the TODO line.

[rsc, 2013-03-11 20:13:15]

What is the relation of this CL to https://codereview.appspot.com/7311069/ ?

[rsc, 2013-03-11 20:51:26]

This CL appears to change only the reader interface, not the raw Decode
methods. The CL 7311069 seems to do both. I suggest we go with that one.

[bradfitz, 2013-06-21 16:16:05]

R=close

Fixed elsewhere.