code review 5975052: encoding/base64: fix panic when input len is not a mult...

encoding/base64: fix panic when input len is not a multiple of 4

Fixes issue 3442.

[dfc, 2012-03-31 10:07:43]

Hello golang-dev@googlegroups.com,

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

[yg, 2012-03-31 10:25:42]

Thanks
On Mar 31, 2012 7:07 PM, <dave@cheney.net> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> encoding/base64: fix panic when input len is not a multiple of 4
>
> Fixed issue 3442.
>
> Please review this at
http://codereview.appspot.com/**5975052/<http://codereview.appspot.com/5975052/>
>
> Affected files:
>  M src/pkg/encoding/base64/**base64.go
>  M src/pkg/encoding/base64/**base64_test.go
>
>
> Index: src/pkg/encoding/base64/**base64.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/base64/**base64.go
> +++ b/src/pkg/encoding/base64/**base64.go
> @@ -212,6 +212,9 @@
>  func (enc *Encoding) decode(dst, src []byte) (n int, end bool, err error)
> {
>        osrc := src
>        for len(src) > 0 && !end {
> +               if len(src) < 4 {
> +                       return n, false, CorruptInputError(len(osrc) -
> len(src))
> +               }
>                // Decode quantum using the base64 alphabet
>                var dbuf [4]byte
>                dlen := 4
> Index: src/pkg/encoding/base64/**base64_test.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/base64/**base64_test.go
> +++ b/src/pkg/encoding/base64/**base64_test.go
> @@ -151,6 +151,8 @@
>                {"AAA=AAAA", 3},
>                {"AAAAA", 4},
>                {"AAAAAA", 4},
> +               {"AA=", 0},
> +               {"AAAAAA=", 4},
>        }
>
>        for _, e := range examples {
>
>
>

[dsymonds, 2012-03-31 11:30:25]

This seems odd.

(1) why doesn't line 225 catch this?
(2) trailing line breaks should be ignored without failing the parse, just
like line breaks in the middle.

[dfc, 2012-03-31 13:07:04]

> This seems odd.

I think it's an edge case that the tests did not cater for; all the invalid
samples were multiples of 4 input bytes, the OP's example was not.

> (1) why doesn't line 225 catch this?

Good point, but even so 233 is trying to check, but it assumes there are two
bytes of padding (ie, AA==).

> (2) trailing line breaks should be ignored without failing the parse, just
> like line breaks in the middle.

What do you mean by trailing like break, the = ?

[dsymonds, 2012-03-31 13:16:56]

On Sun, Apr 1, 2012 at 12:07 AM,  <dave@cheney.net> wrote:

>> (2) trailing line breaks should be ignored without failing the parse,
>> just like line breaks in the middle.
>
> What do you mean by trailing like break, the = ?

The base64 decoder allows \r and \n inside the input to make it easier
to deal with input coming from line-wrapped contexts. That means that
the input may be valid even its length is not a multiple of 4. I think
your change breaks that.

[dfc, 2012-03-31 13:19:48]

Hello golang-dev@googlegroups.com, for.go.yong@gmail.com, dsymonds@golang.org
(cc: golang-dev@googlegroups.com),

Please take another look.

[sougou, 2012-03-31 16:58:39]

It seems like the code will not handle the case where there are line breaks
within the paddings, like "=\n=". Is that considered invalid?

[dsymonds, 2012-03-31 22:56:02]

On Sun, Apr 1, 2012 at 2:58 AM,  <sougou@google.com> wrote:

> It seems like the code will not handle the case where there are line
> breaks within the paddings, like "=\n=". Is that considered invalid?

Yeah, the previous code also broke on that. It's a bit fiddly to fix
that, and I'm not too sure whether it's worth trying, but that's
orthogonal to this CL.

[dfc, 2012-03-31 23:01:29]

I can add some test cases for strings like QQ=\n, but if there are no
objections, I'd like to handle it on a separate CL.

Sent from my iPad

On 01/04/2012, at 8:56, David Symonds <dsymonds@golang.org> wrote:

> On Sun, Apr 1, 2012 at 2:58 AM,  <sougou@google.com> wrote:
> 
>> It seems like the code will not handle the case where there are line
>> breaks within the paddings, like "=\n=". Is that considered invalid?
> 
> Yeah, the previous code also broke on that. It's a bit fiddly to fix
> that, and I'm not too sure whether it's worth trying, but that's
> orthogonal to this CL.

[sougou, 2012-04-01 05:42:12]

Sounds good. The patch works for me.

On Sat, Mar 31, 2012 at 4:01 PM, Dave Cheney <dave@cheney.net> wrote:

> I can add some test cases for strings like QQ=\n, but if there are no
> objections, I'd like to handle it on a separate CL.
>
> Sent from my iPad
>
> On 01/04/2012, at 8:56, David Symonds <dsymonds@golang.org> wrote:
>
> > On Sun, Apr 1, 2012 at 2:58 AM,  <sougou@google.com> wrote:
> >
> >> It seems like the code will not handle the case where there are line
> >> breaks within the paddings, like "=\n=". Is that considered invalid?
> >
> > Yeah, the previous code also broke on that. It's a bit fiddly to fix
> > that, and I'm not too sure whether it's worth trying, but that's
> > orthogonal to this CL.
>

[dfc, 2012-04-02 09:48:31]

It turns out there are already tests for newline characters, these continue to
pass with this CL.

There is an outstanding item to add a test for this string "c3VyZQ=\n=", however
it did not pass before this CL, so can be tackled in a latter CL.

[dfc, 2012-04-02 09:49:20]

Hello golang-dev@googlegroups.com, for.go.yong@gmail.com, dsymonds@golang.org,
sougou@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2012-04-02 12:05:11]

On Mon, Apr 2, 2012 at 7:48 PM,  <dave@cheney.net> wrote:

> It turns out there are already tests for newline characters, these
> continue to pass with this CL.
>
> There is an outstanding item to add a test for this string "c3VyZQ=\n=",
> however it did not pass before this CL, so can be tackled in a latter
> CL.

Yes, I knew both of those things. I wrote the code to handle newlines.  ;-)

[dsymonds, 2012-04-02 12:05:34]

LGTM

If no-one raises objections or alternatives overnight, I'll submit this
tomorrow.

[minux1, 2012-04-02 12:18:23]

http://codereview.appspot.com/5975052/diff/3004/src/pkg/encoding/base64/base6...
File src/pkg/encoding/base64/base64.go (right):

http://codereview.appspot.com/5975052/diff/3004/src/pkg/encoding/base64/base6...
src/pkg/encoding/base64/base64.go:230: if in == '=' && j > 1 && len(src) < 4 {
what's the difference between 'j > 1' and 'j >= 2'?

[dfc, 2012-04-02 12:28:33]

>
http://codereview.appspot.com/5975052/diff/3004/src/pkg/encoding/base64/base6...
> src/pkg/encoding/base64/base64.go:230: if in == '=' && j > 1 && len(src)
> < 4 {
> what's the difference between 'j > 1' and 'j >= 2'?

Logically they are the same. I think the former is clearer but others may
disagree. 

Cheers

Dave 

[rsc, 2012-04-02 16:51:27]

http://codereview.appspot.com/5975052/diff/3004/src/pkg/encoding/base64/base6...
File src/pkg/encoding/base64/base64.go (right):

http://codereview.appspot.com/5975052/diff/3004/src/pkg/encoding/base64/base6...
src/pkg/encoding/base64/base64.go:230: if in == '=' && j > 1 && len(src) < 4 {
On 2012/04/02 12:18:23, minux wrote:
> what's the difference between 'j > 1' and 'j >= 2'?

Indeed.  Please revert this change.  It just causes noise in the repo history.

[dfc, 2012-04-02 20:54:01]

Hello for.go.yong@gmail.com, dsymonds@golang.org, sougou@google.com,
minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2012-04-03 02:08:05]

LGTM

[dsymonds, 2012-04-03 02:14:10]

*** Submitted as http://code.google.com/p/go/source/detail?r=95e67cc5fa08 ***

encoding/base64: fix panic when input len is not a multiple of 4

Fixes issue 3442.

R=for.go.yong, dsymonds, sougou, minux.ma, rsc
CC=golang-dev
http://codereview.appspot.com/5975052

Committer: David Symonds <dsymonds@golang.org>