code review 142710043: encoding/gob: error rather than panic when decoding eno...

encoding/gob: error rather than panic when decoding enormous slices
Fixes issue 8084.

[r, 2014-09-25 21:09:59]

Hello golang-codereviews@googlegroups.com,

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

[ruiu, 2014-09-25 21:36:01]

https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go
File src/encoding/gob/decode.go (right):

https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go#ne...
src/encoding/gob/decode.go:548: if u*size > tooBig || u*size < u {
u*size<u doesn't seem right to detect overflow.
Maybe u*size>tooBig || u>tooBig || size>tooBig is better?

[r, 2014-09-25 21:40:08]

https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go
File src/encoding/gob/decode.go (right):

https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go#ne...
src/encoding/gob/decode.go:548: if u*size > tooBig || u*size < u {
On 2014/09/25 21:36:01, ruiu wrote:
> u*size<u doesn't seem right to detect overflow.
> Maybe u*size>tooBig || u>tooBig || size>tooBig is better?

for unsigned values, if a product makes a value smaller than the original, it
overflowed.

[ruiu, 2014-09-25 21:50:16]

https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go
File src/encoding/gob/decode.go (right):

https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go#ne...
src/encoding/gob/decode.go:548: if u*size > tooBig || u*size < u {
Yes, but it doesn't always smaller than the original, no?
For example http://play.golang.org/p/u2flCMBQ2C

[r, 2014-09-25 22:07:00]

Hello golang-codereviews@googlegroups.com, ruiu@google.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[r, 2014-09-25 22:07:40]

I was trying to avoid the division, but so be it.

On Thu, Sep 25, 2014 at 2:50 PM,  <ruiu@google.com> wrote:
>
> https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go
> File src/encoding/gob/decode.go (right):
>
>
https://codereview.appspot.com/142710043/diff/1/src/encoding/gob/decode.go#ne...
> src/encoding/gob/decode.go:548: if u*size > tooBig || u*size < u {
> Yes, but it doesn't always smaller than the original, no?
> For example http://play.golang.org/p/u2flCMBQ2C
>
> https://codereview.appspot.com/142710043/

[ruiu, 2014-09-25 22:13:16]

LGTM

[r, 2014-09-25 22:18:28]

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

encoding/gob: error rather than panic when decoding enormous slices
Fixes issue 8084.

LGTM=ruiu
R=golang-codereviews, ruiu
CC=golang-codereviews
https://codereview.appspot.com/142710043

[dave_cheney.net, 2014-09-25 23:03:18]

Sorry Rob, this change has broken all the 32bit builds

--- FAIL: TestErrorForHugeSlice (0.00s)
	encoder_test.go:954: decode: expected slice too big error, got %s gob: length
of []int is negative (2207646876162 bytes)
FAIL
FAIL	encoding/gob	0.064s

[r, 2014-09-25 23:38:52]

on it

On Thu, Sep 25, 2014 at 4:03 PM,  <dave@cheney.net> wrote:
> Sorry Rob, this change has broken all the 32bit builds
>
> --- FAIL: TestErrorForHugeSlice (0.00s)
>         encoder_test.go:954: decode: expected slice too big error, got %s
> gob:
> length of []int is negative (2207646876162 bytes)
> FAIL
> FAIL    encoding/gob    0.064s
>
> https://codereview.appspot.com/142710043/