code review 5515050: encoding/gob: fix panic when decoding []byte to incompa...

encoding/gob: fix panic when decoding []byte to incompatible slice types

Fixes issue 2662.

[snaury, 2012-01-06 16:06:14]

Hello golang-dev@googlegroups.com,

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

[rog, 2012-01-06 16:28:45]

http://codereview.appspot.com/5515050/diff/3/src/pkg/encoding/gob/decode.go
File src/pkg/encoding/gob/decode.go (left):

http://codereview.appspot.com/5515050/diff/3/src/pkg/encoding/gob/decode.go#o...
src/pkg/encoding/gob/decode.go:1042: sw = tt.(*sliceType)
wouldn't a simple fix be sufficient? just change this line to:

    sw, _ = tt.(*sliceType)

[snaury, 2012-01-06 16:34:43]

http://codereview.appspot.com/5515050/diff/3/src/pkg/encoding/gob/decode.go
File src/pkg/encoding/gob/decode.go (left):

http://codereview.appspot.com/5515050/diff/3/src/pkg/encoding/gob/decode.go#o...
src/pkg/encoding/gob/decode.go:1042: sw = tt.(*sliceType)
On 2012/01/06 16:28:46, rog wrote:
> wouldn't a simple fix be sufficient? just change this line to:
> 
>     sw, _ = tt.(*sliceType)

The problem is that there was another panic possibly lurking, as in what if wire
== nil? This would real to nil pointer dereference. I'm not sure in which
circumstances this might be possible, but other code paths are checking for
that, so I rearranged slice case to be similar.

[rog, 2012-01-06 17:15:00]

http://codereview.appspot.com/5515050/diff/3/src/pkg/encoding/gob/decode.go
File src/pkg/encoding/gob/decode.go (left):

http://codereview.appspot.com/5515050/diff/3/src/pkg/encoding/gob/decode.go#o...
src/pkg/encoding/gob/decode.go:1042: sw = tt.(*sliceType)
On 2012/01/06 16:34:43, snaury wrote:
> On 2012/01/06 16:28:46, rog wrote:
> > wouldn't a simple fix be sufficient? just change this line to:
> > 
> >     sw, _ = tt.(*sliceType)
> 
> The problem is that there was another panic possibly lurking, as in what if
wire
> == nil? This would real to nil pointer dereference. I'm not sure in which
> circumstances this might be possible, but other code paths are checking for
> that, so I rearranged slice case to be similar.

even if you add a check for wire being ok, the change can still be fairly
minimal, i think, e.g.

var sw *sliceType
if tt, ok := builtinIdToType[fw]; ok {
	sw, _ = tt.(*sliceType)
} else if wire != nil {
	sw = wire.SliceT
}

[snaury, 2012-01-06 17:29:26]

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

Please take another look.

[snaury, 2012-01-09 15:29:01]

On 2012/01/06 17:29:26, snaury wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:rogpeppe@gmail.com (cc:
> mailto:golang-dev@googlegroups.com, mailto:rogpeppe@gmail.com),
> 
> Please take another look.

Is there anything else needed? It's been almost three days, and the patch should
be simple enough now. Weird though, since I didn't see my last ptal on
golang-dev, so I'm reminding just in case.

[r2, 2012-01-09 16:51:33]

On Jan 9, 2012, at 7:29 AM, snaury@gmail.com wrote:

> On 2012/01/06 17:29:26, snaury wrote:
>> Hello mailto:golang-dev@googlegroups.com, mailto:rogpeppe@gmail.com
> (cc:
>> mailto:golang-dev@googlegroups.com, mailto:rogpeppe@gmail.com),
> 
>> Please take another look.
> 
> Is there anything else needed? It's been almost three days, and the
> patch should be simple enough now. Weird though, since I didn't see my
> last ptal on golang-dev, so I'm reminding just in case.

Three days of weekend and holidays for some of us. Please be patient.

-rob

[r, 2012-01-09 20:52:08]

*** Submitted as 0b7f7406d1da ***

encoding/gob: fix panic when decoding []byte to incompatible slice types

Fixes issue 2662.

R=golang-dev, rogpeppe, r, r
CC=golang-dev, r, rogpeppe
http://codereview.appspot.com/5515050

Committer: Rob Pike <r@golang.org>

[r, 2012-01-09 20:52:13]

LGTM