code review 7311069: encoding/base32, encoding/base64: fix issues with decod...

encoding/base32, encoding/base64: fix issues with decoder whitespace handling

Adds a new reader to filter newlines, which fixes errors seen in the
decoder chunking code. Found additional issues with whitespace handling
after the first padding character.
Fixes issue 4779.

[pkwarren, 2013-02-10 17:46:15]

Hello golang-dev@googlegroups.com,

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

[minux1, 2013-02-10 20:40:29]

https://codereview.appspot.com/7311069/diff/2002/src/pkg/encoding/base64/base...
File src/pkg/encoding/base64/base64.go (right):

https://codereview.appspot.com/7311069/diff/2002/src/pkg/encoding/base64/base...
src/pkg/encoding/base64/base64.go:227: // Ignore this character.
seems this if is of no use now.

https://codereview.appspot.com/7311069/diff/2002/src/pkg/encoding/base64/base...
src/pkg/encoding/base64/base64.go:237: non_ws_idx := bytes.IndexFunc(src, func(r
rune) bool {
we don't use underscores in Go, please see:
http://golang.org/doc/effective_go.html#mixed-caps

[pkwarren, 2013-02-10 21:14:02]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[pkwarren, 2013-02-10 21:14:29]

https://codereview.appspot.com/7311069/diff/2002/src/pkg/encoding/base64/base...
File src/pkg/encoding/base64/base64.go (right):

https://codereview.appspot.com/7311069/diff/2002/src/pkg/encoding/base64/base...
src/pkg/encoding/base64/base64.go:227: // Ignore this character.
On 2013/02/10 20:40:29, minux wrote:
> seems this if is of no use now.

This is still in use - removing it causes some of the existing tests to fail.

https://codereview.appspot.com/7311069/diff/2002/src/pkg/encoding/base64/base...
src/pkg/encoding/base64/base64.go:237: non_ws_idx := bytes.IndexFunc(src, func(r
rune) bool {
On 2013/02/10 20:40:29, minux wrote:
> we don't use underscores in Go, please see:
> http://golang.org/doc/effective_go.html#mixed-caps

Updated.

[pkwarren, 2013-02-10 21:18:28]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-02-14 19:20:17]

https://codereview.appspot.com/7311069/diff/5/src/pkg/encoding/base64/base64.go
File src/pkg/encoding/base64/base64.go (right):

https://codereview.appspot.com/7311069/diff/5/src/pkg/encoding/base64/base64....
src/pkg/encoding/base64/base64.go:233: nonWsIdx := bytes.IndexFunc(src, func(r
rune) bool {
Why is this necessary if the newlineFilteringReader is already stripping \r and
\n?

[pkwarren, 2013-02-14 20:01:25]

https://codereview.appspot.com/7311069/diff/5/src/pkg/encoding/base64/base64.go
File src/pkg/encoding/base64/base64.go (right):

https://codereview.appspot.com/7311069/diff/5/src/pkg/encoding/base64/base64....
src/pkg/encoding/base64/base64.go:233: nonWsIdx := bytes.IndexFunc(src, func(r
rune) bool {
On 2013/02/14 19:20:17, rsc wrote:
> Why is this necessary if the newlineFilteringReader is already stripping \r
and
> \n?

newlineFilteringReader is only currently used in NewDecoder(*Encoding,
io.Reader). There are other entry points into this function (Decode(dst, src
[]byte) and DecodeString(s string)) which call this method directly. If
preferred, I could wrap the parameters passed to those methods with a
newlineFilteringReader and remove all of this logic to ensure the only entry
points into this function have already removed all whitespace.

My original goal was to fix the reported issue with using a Reader, but after
closer inspection I found additional errors in the whitespace handling in
decode().

[pkwarren, 2013-02-23 22:41:21]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-03-11 20:52:29]

LGTM

Next up, base32. Could you please add it to this same CL?

[pkwarren, 2013-03-11 23:01:22]

Hello minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-03-11 23:10:08]

Commit message first line:

encoding/base32, encoding/base64: fix issues with decoder whitespace
handling

On Mon, Mar 11, 2013 at 4:01 PM, <pkwarren@gmail.com> wrote:

> Hello minux.ma@gmail.com, rsc@golang.org (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
>
https://codereview.appspot.**com/7311069/<https://codereview.appspot.com/7311...
>
> --
>
> ---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...
> .
>
>
>

[pkwarren, 2013-03-11 23:43:56]

Updated first line of commit message.

[rsc, 2013-03-12 03:18:27]

same in base32. otherwise looks good. thanks.

https://codereview.appspot.com/7311069/diff/22001/src/pkg/encoding/base64/bas...
File src/pkg/encoding/base64/base64_test.go (right):

https://codereview.appspot.com/7311069/diff/22001/src/pkg/encoding/base64/bas...
src/pkg/encoding/base64/base64_test.go:300: encoded_no_ws :=
strings.Replace(encoded, "\n", "", -1)
encodedShort

or something without _s

[pkwarren, 2013-03-12 05:14:13]

Hello minux.ma@gmail.com, rsc@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-03-12 05:50:14]

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

encoding/base32, encoding/base64: fix issues with decoder whitespace handling

Adds a new reader to filter newlines, which fixes errors seen in the
decoder chunking code. Found additional issues with whitespace handling
after the first padding character.
Fixes issue 4779.

R=minux.ma, rsc, bradfitz
CC=golang-dev
https://codereview.appspot.com/7311069

Committer: Russ Cox <rsc@golang.org>