code review 6374059: encoding/gob: fix check for short input in slice decode

encoding/gob: fix check for short input in slice decode

[r, 2012-07-11 22:53:52]

Hello golang-dev@googlegroups.com,

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

[dsymonds, 2012-07-11 23:11:51]

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

http://codereview.appspot.com/6374059/diff/1/src/pkg/encoding/gob/decode.go#o...
src/pkg/encoding/gob/decode.go:655: if nr > uint64(state.b.Len()) {
I'm nervous about removing this. This was originally added as a DoS protection
mechanism to stop a message of only a few bytes causing this code to allocate
many GB.

I still don't understand what goes wrong here. If there's a type definition
after a slice then this condition still needs to hold, right? This function is
going to require at least nr bytes.

[r, 2012-07-11 23:35:45]

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

http://codereview.appspot.com/6374059/diff/1/src/pkg/encoding/gob/decode.go#o...
src/pkg/encoding/gob/decode.go:655: if nr > uint64(state.b.Len()) {
you can be nervous about removing it but the test is incorrect and that's what's
wrong. in the case in question, we have a slice of interfaces. the first element
of the slice defines a type, and each type definition is sent as a separate
message. thus the length being looked at here is the the length of the type
definition, not of the data that follows it as a separate length-delimited
message.

[dsymonds, 2012-07-11 23:41:09]

On Thu, Jul 12, 2012 at 9:35 AM,  <r@golang.org> wrote:

> you can be nervous about removing it but the test is incorrect and
> that's what's wrong. in the case in question, we have a slice of
> interfaces. the first element of the slice defines a type, and each type
> definition is sent as a separate message. thus the length being looked
> at here is the the length of the type definition, not of the data that
> follows it as a separate length-delimited message.

Aah, so can this check just be skipped for type definitions? And
perhaps we can have some sensible upper bound for them (say, 1 KB)?

[r2, 2012-07-11 23:44:58]

On Jul 11, 2012, at 4:41 PM, David Symonds wrote:

> On Thu, Jul 12, 2012 at 9:35 AM,  <r@golang.org> wrote:
> 
>> you can be nervous about removing it but the test is incorrect and
>> that's what's wrong. in the case in question, we have a slice of
>> interfaces. the first element of the slice defines a type, and each type
>> definition is sent as a separate message. thus the length being looked
>> at here is the the length of the type definition, not of the data that
>> follows it as a separate length-delimited message.
> 
> Aah, so can this check just be skipped for type definitions? And
> perhaps we can have some sensible upper bound for them (say, 1 KB)?

We can't tell if there's a type definition embedded somewhere in this slice
element's encoding. In the original example, the type definition starts 6 bytes
into the element encoding.

-rob

[dsymonds, 2012-07-11 23:48:58]

LGTM

[nigeltao, 2012-07-12 01:03:12]

LGTM.

http://codereview.appspot.com/6374059/diff/1/src/pkg/encoding/gob/encoder_tes...
File src/pkg/encoding/gob/encoder_test.go (right):

http://codereview.appspot.com/6374059/diff/1/src/pkg/encoding/gob/encoder_tes...
src/pkg/encoding/gob/encoder_test.go:825: func Test29ElementSlice(t *testing.T)
{
s/29/Long/ ?

[r, 2012-07-12 17:23:58]

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

encoding/gob: fix check for short input in slice decode

R=golang-dev, dsymonds, r, nigeltao
CC=golang-dev
http://codereview.appspot.com/6374059