code review 7486051: image/gif: handle invalid GIFs more cautiously (issue 7486051)

137 views
Skip to first unread message

jeff....@gmail.com

unread,
Mar 14, 2013, 11:26:47 AM3/14/13
to nige...@golang.org, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
Reviewers: nigeltao,

Message:
Hello nige...@golang.org (cc: golan...@googlegroups.com),

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


Description:
image/gif: handle invalid GIFs more cautiously

GIFs with huge bounds and not enough pixels to
match were causing huge allocations before giving
an error. Now we check that the number of pixels
available matches the bounds before allocating the
image.

Fixes issue 5050.

Please review this at https://codereview.appspot.com/7486051/

Affected files:
M src/pkg/image/decode_test.go
M src/pkg/image/gif/reader.go
A src/pkg/image/testdata/issue5050.gif


Index: src/pkg/image/decode_test.go
===================================================================
--- a/src/pkg/image/decode_test.go
+++ b/src/pkg/image/decode_test.go
@@ -24,11 +24,16 @@

var imageTests = []imageTest{
{"testdata/video-001.png", "testdata/video-001.png", 0},
+
// GIF images are restricted to a 256-color palette and the conversion
// to GIF loses significant image quality.
{"testdata/video-001.png", "testdata/video-001.gif", 64 << 8},
{"testdata/video-001.png", "testdata/video-001.interlaced.gif", 64 << 8},
{"testdata/video-001.png", "testdata/video-001.5bpp.gif", 128 << 8},
+ // Images with invalid bounds should not cause huge allocations.
+ // This image from https://bugzilla.mozilla.org/show_bug.cgi?id=525326
+ {"", "testdata/issue5050.gif", 64 << 8},
+
// JPEG is a lossy format and hence needs a non-zero tolerance.
{"testdata/video-001.png", "testdata/video-001.jpeg", 8 << 8},
{"testdata/video-001.png", "testdata/video-001.progressive.jpeg", 8 << 8},
@@ -78,7 +83,7 @@
loop:
for _, it := range imageTests {
g := golden[it.goldenFilename]
- if g == nil {
+ if g == nil && it.goldenFilename != "" {
var err error
g, _, err = decode(it.goldenFilename)
if err != nil {
@@ -88,9 +93,16 @@
golden[it.goldenFilename] = g
}
m, imageFormat, err := decode(it.filename)
- if err != nil {
- t.Errorf("%s: %v", it.filename, err)
+ if g == nil {
+ if err == nil {
+ t.Errorf("%s: expected error, got success", it.filename)
+ }
continue loop
+ } else {
+ if err != nil {
+ t.Errorf("%s: %v", it.filename, err)
+ continue loop
+ }
}
b := g.Bounds()
if !b.Eq(m.Bounds()) {
Index: src/pkg/image/gif/reader.go
===================================================================
--- a/src/pkg/image/gif/reader.go
+++ b/src/pkg/image/gif/reader.go
@@ -15,6 +15,7 @@
"image"
"image/color"
"io"
+ "io/ioutil"
)

// If the io.Reader does not also have ReadByte, then decode will
introduce its own buffering.
@@ -154,20 +155,18 @@
err = d.readExtension()

case sImageDescriptor:
- var m *image.Paletted
- m, err = d.newImageFromDescriptor()
+ bounds, err := d.boundsFromDescriptor()
if err != nil {
break
}
+ pal := d.globalColorMap
if d.imageFields&fColorMapFollows != 0 {
- m.Palette, err = d.readColorMap()
+ pal, err = d.readColorMap()
if err != nil {
break
}
// TODO: do we set transparency in this map too? That would be
- // d.setTransparency(m.Palette)
- } else {
- m.Palette = d.globalColorMap
+ // d.setTransparency(pal)
}
var litWidth uint8
litWidth, err = d.r.ReadByte()
@@ -179,10 +178,19 @@
}
// A wonderfully Go-like piece of magic.
lzwr := lzw.NewReader(&blockReader{r: d.r}, lzw.LSB, int(litWidth))
- if _, err = io.ReadFull(lzwr, m.Pix); err != nil {
+
+ var pix []byte
+ if pix, err = ioutil.ReadAll(lzwr); err != nil {
break
}

+ pxExpected := bounds.Dx() * bounds.Dy()
+ if len(pix) < pxExpected {
+ return fmt.Errorf("gif: not enough pixels")
+ }
+ m := image.NewPaletted(bounds, pal)
+ m.Pix = pix
+
// There should be a "0" block remaining; drain that.
c, err = d.r.ReadByte()
if err != nil {
@@ -325,16 +333,16 @@
}
}

-func (d *decoder) newImageFromDescriptor() (*image.Paletted, error) {
+func (d *decoder) boundsFromDescriptor() (image.Rectangle, error) {
if _, err := io.ReadFull(d.r, d.tmp[0:9]); err != nil {
- return nil, fmt.Errorf("gif: can't read image descriptor: %s", err)
+ return image.ZR, fmt.Errorf("gif: can't read image descriptor: %s", err)
}
left := int(d.tmp[0]) + int(d.tmp[1])<<8
top := int(d.tmp[2]) + int(d.tmp[3])<<8
width := int(d.tmp[4]) + int(d.tmp[5])<<8
height := int(d.tmp[6]) + int(d.tmp[7])<<8
d.imageFields = d.tmp[8]
- return image.NewPaletted(image.Rect(left, top, left+width, top+height),
nil), nil
+ return image.Rect(left, top, left+width, top+height), nil
}

func (d *decoder) readBlock() (int, error) {
Index: src/pkg/image/testdata/issue5050.gif
===================================================================
new file mode 100644
index
0000000000000000000000000000000000000000..8dd703a62ba14b25ea8aba1a577fef933737ae2f
GIT binary patch
literal 11606
zc$`()XH*kQ!0_=+HVp_#=%E@AkZzEUhym$RMIfMJ=v4vfh?-DR=)DOF7Mg_)LAe@w
z*PtRrRGJ7XUhG)Nee~Y*mgmEM*t1{eoY|Q(zyDa<S{WMo_yd-}w|@XWC@78qC7}`$
z;&>@J`8}dARUv+5h_WHV&{z?wrzNeXtYoaGLDbYa2$-2z*;%0gTQLE99jKEr+D>m@
z;y#sAcz#=mtvldhBYpTJKylWz3xEW~1Mv|+r0XHEBMt_M?$8KZq@Sy*m6fHli<_Ib
zUxcemu%As_gu|JbsACkj=m;8c833*Wz+Zr54S?^4;_gWsTnBX6fXsb$BnK%uC@H~F
zQ65&&pU~ItH`ML3(H}Zu!v@IpfNL8nmJNir1LhCS4^JKToc6TncPEd<x(o(}&V?pC
zPY4`NikP%b%E$_RnCbl>B~w|jz(AcQhe`2<mAQZ!fzXoE@PbHeT{75_1eEy8W+oj<
zPjLSU13!z4eHB4}RKsqntG+Wb`lPz&lfM3@p~_?|?41+(T@?SDD76nqHRqy(-&$M0
z@o?Wce3(ZjZya@a8x;61A)Xf$#EXyLOgg<3>`Dh1r9k;*pt=cYz7KF70Z-Dqfub&G
z2NUedLDpqzUhe|#w!yBp6UwuW<!6W17do*xKnn*LEkn#)1jkEpW97t=ZeZ>@^jW>=
z3>$dbp#1pO-mm4bw*|=e`NXvw;M*?fP6OoU4bh!$$gfTbUf1rO9=)|Tjrlg4X?Dm)
zO48fxsExZ$pKiv@rrFQ-0V4yz#whTnAI}?u@P<Wsj{)A;?wtwUjX{m)0}f9|qkfIp
z{u=f8XUdlM*!z92^~PAz&O`w3iSN!#;+Kg~W`0(A<E_@_!rHbwnN`<r^z_l6<mPd2
zFqc?Gt3|mhdAA<3>o)4z-?g{SRdqj|?0&q^Iy!!LZmILt?EP1--!U0)ONx0+Ca<jQ
zT`~PvUEL0w{jTZuo9k(lLs<_WcJStyJI@L?9<e^WXyGkY?JU>rtlt@%eK!C6#n_|K
z(HARoFP3}%?j8RyKKEsId}V%YX?=X-)7<i_`OQx+KK<Mo8R5;$tj$gGHb;1$=H9=Z
zU0r+q`u*0%_l-B7zwy@BdEeG~Jl@NfFXg|(;LgB5fc!rKj+RGV51Bbz?$aWI5C8x)
zem}sUWdfjhzz&~`O>=c`DhfMxv9!6SpMl$Lyt*g%^8Jio%_nZGM_e+w@XO?0LmYSa
zA(o13qfEl&<WT;Tg^_Fhxfs0*)4@mX>SHhVgiZhOIlHnpIZWHR?Q<heb9FdzdhNqX
zcWYyRVnp~o*7v|1`|z0c&sX&s3I}hWUZ1#l=(W_d^I(gx!_T#BtJ3KOnU1j;%8Oeu
z3rF{>&(<CcjX?|@JsfrInZX`2;p;1UM7^6u<6ocK<KD_7Mib&Eq`uxH(-K?wiacT*
z^}-|JAD<<OpV=r&F?#!TCIZtjm$c(^_^^14?~~rW^I}Pfi{S-E*oQ)D(SM)5*A{_@
zbNK8vJ_df`$-OdQobt$Qwf=F}s%^e<s-o%JiNi-;EVRq_yYFVruagpE`~zeY6<Z97
z9lTHM-W7C&Pd7Jywt5(Qe4Qp*9<8K3fK3X^KDv}bpwCj6Dv4>6>4E`iSEAMJbe;Uo
zd#!xU$fLT$wja0CybsEI4pf991i#QzY^uJvSUHG|I_n*i^?bq5@l7uaJ~V3a$OK}_
zn$~L(y7tn%$?cGXcPe~sDEoANfQPqb@7L9=s(|a>5z&-Gg8qpe9aB~J&dugJpanfd
zWj|`@79H8$VkB8;-}p+pGRn(nzN}_v)hgPp@wH6hiNu&dOEOgN2-UAP-67F_?2!1m
zyrF5gV4g{Ahlo}DBf8jy2La!O!VFZcC#zz5zE(>&<_{`f4uj0Sx%KjS#Z-{Ax#hbO
zY2l)xz^@<TZ=BuBn=c5>8q@W+cDC~LIB`5MQMu&Qb5EgT%vjaR3pYd5%g&wX)?KiU
zUvqnHnb!G+;xV07`Z(Yho1qnPeCf_S`({pUMb#<DFCpiHHOPwAysw5X9j#)axTAX<
zR4$G}V^y2X)CEkqrlJAS4*H*q1eQ$=1=@4Rksm;eUR3iET=Ha^5Cd;J)|G2?ubGm<
zr&P4n&hN0jJh6YjfJUyaZ_JdXbW}_)Li$XzJYC}Ylj!HB8vaxWQg+~+^@1EF5&{)0
z;->M8JTp}o65f{&AS6#@O(70OwMQY8Dps%P2XP`Jk#a@xR1!jVF`prq^sa{S3RdQf
zVeZ)LSV^1&<dBQ$<MId@?-3zKtt$UVEQ89HYBQwV=l2{Wjbhp@mn`}ai3IXUxyh?7
zQO44b@TQ2jCkBLuRlY2^J~Zr!EU?wmtQ$xHC5kTeLhf_hXb8ClZ6mqp-B<wCI(t=m
z`DE8phlJ0_W4cP+Tc~8aOYUa2pUm<Wea}9#%q`nAX;DKepg0R8m0g8kI8?-DTo(vX
zAkb|<MQ*tC{77$@iYP?}@eSUTT_ehewuYKbp^m&;7JC`Jc<4udrd@aqjHgKM-8<AP
zh_K%RY(%NT2ia&b7%5Hh=9r%I2%<062NS++uHVN^-&b@+x}|JjPn>q<q0hboeleEH
zcU;80{rf~-07$KbscaZ81qK3Cxo;>yj0(!o@f?^{aB(6IgYnC=lI|fLc-<f`akt)*
zfc9ZJ+4#al$yD(evrLCbg$$t$`exjc;0gO>TsGIzFwQLF&`l~tfB=9bXJ$mgN(rr!
zq!P297m_fUWLAifK2neHTJICTjEC?^Im2xf3BNZnGtKa&zdWy9lk2!RsFeWW6RoET
z6qZ@;cD5BbK4LD;o=rEN>CQAqlOWH?d<Rjl)AieXcP;XQ56w6SvAnk1#2=)s!L;O|
z2k47pZHsAphH1iH{^la>BZC?@iBvd|DwMUgCa&lsaBO|KOm=hKI<W-~5IHhxy;cGu
zqx6sw2*w9YRXYA5qvGDO2pmpD9qmQ&DUyWr0tS`!eFRupheSFr4r-Wa!vPopl8G|2
z6yq>2cuhC9Zw;XA-((fpgBTy6QbJgwQzy7jEZ9<Ao%{jCCkhCJ4Gt)gX-{jC3QA$4
zaHV~{d`<~hL@uC6KrM%bTi1r<H8=Q5jJv~uMaXxOtqetq(R>?+2^tyjGHQAHpmkX|
zgPb0;|1SUC3{c@~{qsZnr}!Kr6$Ho}9!$O2nF^3;w?igRo%U?WAOTe19TPRNu+<U5
zG)JG@PLbT_GVf3nC&m@qRP)k4;qGj3C+;~+dY3|uy}xf$MWkksK`(zg)Lf`l_LTa~
zuoE7rSg}g3IscERdErM}(tQAFX%2~1V`^|`>eOItlK|!28>qsS&!?63m!7{9$8(;X
zXrjUw7Tm5aizR$Pnm*_>-s^uzD!`^cW%I|$b*r*I@cfoq#t+;P6?X6LxZ4+Y$t-P{
zCe-84kC^mv@B8E|p^KR<S#BmiqB991Wq38sI5tOO|7<s0ygAy{p0a+rPpoHkn6ZBW
zPOPr!<)!b7ai#I?cKS8|bsXd=<@K=0D!V1em~EA>d(hKr+oOQvV7WGp1w6xZ;w?u^
zZ!fKc6NLrOo`}r4y#I;fx9*V(-sx(Jv~+6=51f8;uYm4f8xk=;;M&Zzl)Wu$L~Bn@
zX>&X4l>O3rt3vWlp4FMZ2=oxbr0`zv_OhCvD0K=1SHMhf4rmcP-E)=CZM$uk$qrO<
zs~Wj~u~#UE3(Cw=aQxmDVsiPbCtMRmF4UWs?@3;BKQtruRmVpEbLkyp``z~y5N!>+
zelg}vt0fJ`yL3p^q3ej=$5tf7{!2oF@vHl#90>!N&+ouJsmmhG&gtq$Ap)f2#X5-c
zx76t4dFRt6!~93A0A4TAH}O!yG#U2}Op!<X!1`ez3KDbcE<a(5!nYc1y?=QL`{VtJ
zv{DlkkfQ#bybkXAo5R?Ppd53*nmEn91vfX9qlec&Nw+!o@wVq*EF8+cxkHcyF%R=~
z(@2$B4@R0qmgIJR+P%vVw0>&P!h|V4l-Qh|Gp+nU1=1)BL4jW`H{2CSo7gQqN3qfO
zBD0V9#}Azk)Kj(qAOc6Lp(U477X4gu&QyYO2ZoWR`ie<YFa2n7lTa7|2jQ<={31jI
zM1hklHqXk$(C3&VUxw*XDo5Yy(zTT9fpk5luV5`Zjdd(PBcOWa(|3-wk;Tizjbppk
z=k!uVukdhi6$>+d@#?;-@Q78B`w9y5Kv3pd3)VnCe(2agI?8H3bi1Rz3nE|piKh3l
z$<-^eRR-rEWXw7VTeX!?OyRpq;D1U%26BUFU7?ol{;fEnj~v7tQSfm!)*i>V4-s%I
z-nXkX3L3|+y9Ilnhy4eMeYA(Vx|Q;pig`s9a8mXtioBp3czVLapH&UM1%y5m5K#o=
zCklEm0K;vc@AU{!oaHBNK~=Ud?0B5pCSfAKCBJsYED%w?`u?eV5`@YA4U3?DH4N8_
z{a!5iHV?cc4*MXHC~T9`Kt*Y9K?k-l>%{c!X6*M!4EZtk=OSa3ir&9SQxi|-Q4D<=
zaIo??@ZdHsiXgZXiM~`4a&C*BT9;}`N0kSHBUH?L!Z~sw_V4QGZQQwFYs{K6XbRBm
z*Msa0X-sa2zB4*#HrkjIerh{d!RE9`Ju|C5M4yQM8^G>k>V(cjKW1Q7BQdMa7^^L)
zDTNuCaOVCjf2B&+UJ^_=A5&hN`Dh*GkdQ4x=Qjm%Y&a=LnApw5I8QR>0T*4xfL*cz
zQ=5}l6S5l!u*GetK{Zw=A6+>Va!DLCC*-`c%BjP_Vo^B<C|T=~8Lz72t_-6`7GVQi
z^g0J^!U@;L!ETr3n%IEF?2xK5jF1l+HX5$QEg)`zDT_Rmtzlq&9_k<fYR(nhVS-%l
zg*W(AHzsV~BGiBav*zYs{tDIQBJ=a|?w9eWY=MHRSztL@bu_&4X@O>Qu@)CfB!UNV
zB|b)=;T-ftNQv=ixXB`HlZkmiLJttpR>ZuWvO=xz;5j0y4#(FJ$zQjKVi90E)DokH
z%!<HbTn!r55E2(sdMu<QZLTcjdx-ub?DZo0KqR=2n``?WYV^G<mk+$6S`o}w5$suT
zu@)`Vf-&HhUs5h_o+~5s<>*tvTOnnYqXj`B6_>x4A2%wmT4M_5!|KZ~YAu$J@Kv^l
zl+D|ele-HpNnU)ESUJq9u%rr)MP7W#V;6ACFSe6eFSo0fB`>WsWE$I6jm+h$mxFKe
zga^4*Y*s~#<mI-x@=uAEl{cy;HY$v(D^zQ+cEIIK&q{Ev@>zM&YDo1*&jS9&>`%YP
zS`Eu(3y)TV&l77t7*$IeU&PK=e%+`bCxRA{XpvB{A9-k7BFs1sG~lv+bXRj5s(*%5
z%Z-)mM}o%AFuOdo(jDm^&6qcUU_=~xFb`%)f~i%28uM4Qeq4F5aZNc9G{^&uNibVy
zbYv$67}Y`4U=Q+N+t>;mC0(=2gIOhAvoVILHG<)dU}$5l1qo*94706<**T+!fuN0Q
zjQ7}e0yK~M=6cE-Ec~1r5ds1J_(=2*hW<dP@}Il^P=BN8_d677@!!XNzwx9Y1>gWX
zKo4O2n-gG?us=EB-<BYt3{^6M8yPD?^|ho`l@xXNYDvk-n^@UdV-Z$ZU1xo$y`FxY
z&fZgaemmf>8_LOt&p8Mh6a&OYg1^tp5q?B_4-2fzVcq{KLf~P8q_BflR+jGe_R+46
zQ9;&GXU@2vJRW_Tip~)SE(6ePq-c@)|7!_%{<H+$RzR*^9l=3J4N6IHR1}By7)%&w
z_Z#XBIM}iQk9y!pD}UUbKNZ2gHRg0rocTj@`$zU((_R++?ry`U;-ANdOh>3+J(j@A
z3~fmdeV7?ARi3FVn4?F$tE+oiMzm_LSEeT{FC0~u4Bm;tRfehLI=eFC-FvL9{_=2R
zM;^Ho6_s-C?3AhLWGw7qu=4z|W8+Rv)8UR2VWC5DaZiJSX5&I;<Kuf1gWh<0?%3P^
z^6=QOw*1@g*jiZFtGKxDfq}o`<5?i93@A^>(YgS3wd%!t0B0B&xfO6R{m!43(3t_N
zW&f!NjTOoz^dm)#*rr0Kp1km$+Qk3AB@E@^#*4M*u0#J+gj?8|M&;=y)wiYme-c6`
zxYGdqx+V6j8}hSDY=^D5)1$Z4rTj4OF!!e0ldi~BR`~xZ2N2#kWM|}0Hu#ee#&`23
z|93eU`BM&d9((Uh`thFl?aU;;8I5J-=9k}Sy3b(TFD<>-*l@42qp`KE?pD=}o<7E8
zdCJR_{HYuDkJ&dLwza-(t=p)pc~R2SKX~`)LffOs?q{z$U(I%`zZ+t)1}Z9^6lIUK
z=k+(#53<?QwY9U|U47lPYfQ!toz5#QeN~vdeE057ef<wMd%3!La)dQC-m<fhw>8Ds
zeA@KmRrjyux{;xwvAOx_;o;f2xkt0Z{}zQM?$+bc<@M3GU;YaTbDw_>eE#rmWMp+_
z=GWNRuesUfX9FMJEUf+864tjj-hAHv`Fd^V>-x^l&dSTB-zx*f1G=*2jwnCacYJ!j
zy`eMWl1$i+g4Ke7bxvjC&RR>uQ>R@WM$r@OazBS>J>lW)Uv7NO%P!YEc4^c_wI$FZ
zSwBq8zkg`5!jK)wdLkZSFA@A9^yB!`z>`+`xbIayyOm$GCjM-6JJj&VOFGQ&g-T_c
zz4rs=W|`)=lFP34<J|Wj!uzcRI(4Rut^CA;vIlj#CzkrVa_8_m6BEJOZB8ep^uxYx
zzHOyD=$}&c80$tb_P7~Ne16_v*R;!&%`NH32eV(Y10pp{Yv<D?@9WzBbou;lXhJ7(
zb3(iDP}j$@j#qNyb4bnX%5cfo+_$BZQf+!+jh4s6Pf6>9Uhd=#U)n8rIW%~stXO?G
zT+Jyo^s?q%2_YqEqY!+Z`b_R6nM)_i+Aitt*oYvd15ElSbQ~>@R#>nXj$o2L+LN;-
z14de`B)X0ljcZPn?S81)RoQ~pxun{Tkoi<J^ibxTgTcdnL6K>a(m|Bni#luMLzVam
zQa@UMh3iph`U90do-(x{GNB{Ox9zU`%hvUQ;nX!hx_*J%ilnr2%+#XkKQ5_)x-(oQ
z>kBdEG>3yLD7>|FAilv#H)SK$Uem6U*oPuhS|EJnn3}`d?9b7;Cf%I$VEvGyFIke2
zCL-CnC(lFD#cjXGzts7$aK!4mV`3sg>!D5IxZyL4&<8rahegp&M%&fSW^f4#VGw`B
zKe87t79Pog?pCQ8-}mc_tD{MMQt;5e`_Bu;kDn>3Nf!_P{8~<@+j*nNXhnC1j_28<
zvW*7AT_q2!<@x1kNpM|;dya%Vn;~LnTaa>zPilH0XiCjti<Ma&N24=baMEFEx!+Z4
zD)<xJ=@S>U!p|8h<?25@n(Cmvvf5`a^<9;XHJaJ7cxWh8lc$d8K0;*bkZt7CW#iXt
zkaF>4f*JBP$8%P4{DA9_=2|sU(8;B!WkS3-q~B?e&(TA<rjL4h5i$iKZEp+*W$==*
z;qARzy4<oz03Pm2;SipOQ)qDf#9@YP%r=_DLtct$!)p0Xv^>=D9cfWJz}{LNl0Hto
zija9MIfRg`D)JkbeC9lZkeM$MaR@s8vQ02&cB0MbfH$5%eIxMXu>%Zyl++6k7dT6W
z@uRsA`H6r8tbX7^SC&y%^5~!eeHD>IKAkqjI?!c<xQY+fDU^#DG)eW7VlA~g87O5D
z`uOgqefseRQLn{;LPZI^y2>yv?EtH)WlJx(>Gg}y%eYx515ctE-w41kNb8k+nIb5I
zTfqj0`|VmoA#{0d=;X?)lFun=dj>uTx+U~WFIT6-5CByeN`U|<5^6v`$Bamo^H2e$
zCZ@8_sH{tdF#GXbXQsLRZV4Wn08=!^LEz3cB2{GA?n-UUh0nV~x`KyvxH_21^;F3K
z6;OwN32imfC*4eeN#G#@5iuzO5E&X&w3&)2<B04MWe|cPDdr~No^8B!<?=v=N4u|x
z>^cWp&qcvt03f%%TUl)?qit)&%&1{l!BbwqlXL}UHqxg|7Gi3ku83SB52^*aj2>}6
zf`i*obsjvzdayqV(P02g?gLFY2@t!<8Bih$Vcc6=#fqCP)V+H7)kLY;nOleWU?@KF
zz;byLbxr+>q<|Tk0?`=fQ|k=L@EQ{q$)y5N;TDG@$CrhhwgMdcd{}n=DPlKCgQ9j!
zs<pj*F^)%0Q~x&l@LcVc5&=UiN1Lb&*E|Iw9DyW;vSZoirBvVP8BmqF_JGN`BHa5V
zO?lrE0}7+?*KPp^cC}0vRJ32Kqgp@#E>QVowFWCYs3;0~Tv6aF`no60P|Y~sp7KPv
z9M!1Q=nS{ZyTT740AfTJZmn@g!x|a};7OQ^S-zqMe}$^HCQaY={!-bx7;><WHg(yz
zPq>V$q&(5hw7`F<)jzlH9&zWeS`knnG}Y*FILhtFR>zG}j;h9hT4rmBUft`@jlN&T
zX6~Pl6}`F0aFcvCJBZM4-dI&73TDoZYsEF?oO@^FBtKX2E>`q<aCoq_87n%_z3L~I
zH{hdP;aZ9K+E&-0p=zftWcN$2gX5-oa!<ai8)}6;;_yC-G3EH+>DSJ>Q!0L9(zC4j
zYaR81?@wC#WY`?75%Lg&D;y_*QtZW^HO+`bA1<@iE``lXKkdN~@V?df^hW9IEjMYi
zbhT}H;VS2JHTR{g(>D+C9o-7u_24a3dc7KD%%ursRfEzT05|D2ASz|XS6T6?{d{#w
zgzLo<i%G@pg<n7jlmI<LXwdz!Xo-At)@@g17m7n*XcsME@O!TEU8vqv97s%et5*=d
zNJv*b(!zHzl2X}cdCU852LFq-FO6<0HK^by^a<`V482A5u$%&A2{f4v@-T0|%u$ik
zN0)^oF1?24En$)ucJKNYY^`ZiMp;65jCsUbI$qPrP=yfyIG$5eSiyW*p@3hrK^nhV
z$WML;?)r|mHxy5Fv?Nd>I!MErnNw7NX>^x89Y)?W?qQeAfnf4DBFY4s>O{O~V=&{|
zTn%>r%as`puOmF#3r^lofGQGt5tJR8ZQgcQy}PDrQiw>flP|xEVgH^-9G2~@hv-7^
z@HNX7?1hE?g>}<!8ZRnc)eV>Uw$7)BR1)D49^&)gw9?gYCI|fGZ7xIsQWcG577l*)
zm3=>Ay<eOcmUwNqEDz_h|4lyYfnAEw<!zYW2tdbN)_<wwJ?P1M_tx!p30{;)gzn#J
ziHwIKmp2uV6_ZQ3u^05}@WmENr#EM(10{qEorkLrnx0%NC`Z^wGK}Vza-uK(lv^ic
zvuE;iD8O<jdH7WD*-Kt7@2Cjt5!(P;@8u_QS9<PKlbR-@f|9GR%3hDU%ckY0T9G{N
zbt#{nJM7~pu|Qg{)%?+xdiLu_XzIo{YC_Xg3nnIOO)j|PyI|bn=Fv~de>ZIYOgyLU
zIU)TVxe;r&cVX&tjLmadSsMGG|NGktYzi``*`&HdV4<&Y27ahse=}7&Lw4a_j}>`%
z+*0IAugQYU5OL^HcBHo?0ByfpcR+=d>;35kVV553ylO2E3nJ<~keh4<WlMge_U8(o
zLopXAI*5@a;t|QrzrKd#q9lpiUsvJD7MBkEo|I**$re<TNd0CF>gS;~k+G||^J^5C
z-Xc_+=wlRlVR9G=t2?*c0TQ{;^=X<e5v|t??&pAtD&XE~s2Ufl!c39bK8Ku%8Qv1O
z&lR{G@BQ4GrbVQxY(q`BNGiZjLh;vZT{uVp!Gtu~gtG({A@$ib)%p~h^>eF(u+>Pk
zSfIC|4NYx5O?4aUy`8SXPIn=DuPUMS@>86cbn6kCOj+8i08o#bPA*GF*QPooq&c$F
zeP-!oGWcT#8(x<BdYUe3$atfa&YNa1qiv{RZ2DOnYRWpzf+7MhORuq}Z<l)4FfPcI
zrKN7OXd`q@3KRV`Uf+x!UB+BhV%(_DXsBmi{5^;~sD^W<1`)JvhLM~zRU<DLQs_n7
zXDilO*G5=p60%N@u#DJH5)QqZhaSUa|J@w;cQx991J!f}t*PgXgfdH+8O^g9^)?x0
zOqOOdRD&yUxDMSz_Yj6)594%iS}-b@S+{~&x9c-B^PnWI04FbSYzu=;QM~3OmAi_i
zFCM-Z39}%=%&DM(Gbr5P-NZ~4vt^)5p!c0muW@p;9)+c8<4a3IelDWlkJx?BD|l0l
z8P`J(H0N1UU0@d6Jd7#~%K{Bqpngc<AtFqN1+$KXnc-l#Qp^Z9r?;f2_p9J@g1e)q
zi0T|x)u{N0XR(nshC{%v<MM2lV|WDY{n{jNKFni|-T!sx|Dq-Mhn9G>#BW!s2n#5K
ziu+-PhKiuJy39T;_1~z3C-DoM75+m@A-><Pbk@_3(IXzm@38~y$WV_XB9{NUGA0@q
z7%1=LWT%(l4h^$`hq%dESy_47JDzcJiV1Nzb0*r?!w1NK09An4MalmliT+FSUrT~w
zcVu*L0}>5NIxUvaUQlX4N}^Bew<T2vHTMlTsZRL($4TcV{>Uz9OqsgNHMg*90snE*
z<$>psfso&pjCdLzS-}c^kQMWy$z!(gHzbSnHLvUF(Bv=|^^RwFfviAiaU?1?LA&nk
zpG)a0G1`}0RI5C_itHT<Vq7YNgR2taGlGH|yu8@9HeK#+4fZzIf*k9Eg09EM-w6%r
zPE4#xh-h#P?ETl9ug;1NSzC{|yFatFo$>G(4+|Meh@TD!coZAC;o<Qcn>+6Ae>*z9
z3JCZa5b!HDb|W!yF3Oq#W0V4A=^`mjVAC)#T5=}sH#4(ugB>hXQ?_PHC-l;FiHy`U
z^*N5$i{pCgj@_-XZ>>F(RTcgpHV2CkGv%<sBB6&B>hss(vuwzdn<7tdD9yC%yshB-
z51YS!WAk5Wer}fd#a8-wci+S<=SN*pD`hADW9Ir8@GmpPelzoZpVZE{;?9Krr?I^Y
z19pFyIcD=~l>E<>4ezn%+kTIoi9gc(HIuM8dZvoSswyeTDJ-gEFl)=o*bGK{Sz+tV
z?1tLfj{5rU&d$cpmXi90rhC2VPqT9$-e`Q#{vSCntrT}W<g%Zwv~Ii}q|=8>N}n<r
zbLC~jwbzHcyJl)@pLKTjci(=)r0*~ozlw`DDoTGdlh@Vty19wdSvozO^<=VTXP)tK
zI{V{`TRW@uJFmO`St=MA9v+>Uof#gUn}7D;>DZI!f229LvNrwx)4QRemHD|JqoY4(
zW|kHPKWr}i#^w6AjW=Ju|M+uF{rWd87vFCEN6Y>^`9rr(?mdyPHaq3s^rG_gA6mu_
z=g21QtdCAT#h$#w?0Y&pw|2|h!05$~5NTDUeVN8zFRm5Yv|Toh(~Gt$`gW?Ko#^&7
z`PgfCEwNJQdu&_V-j&KzvQ3+Xoto$a8S{>{3tyj%_auG%$|q8C+WSGwUyU0-X0^+-
zPV_5|xtiY+kT!}-{!&o4cUjNd`oj`fp(L>svweGXW#68PGDECh?Qy<SvT`~hoyj7B
z;rRW%n=d|QxHnyG@xJ#|Z*b3)S7~s-@<@K$t%&<!$2<KSbWVjoPI|dOWewf%FitwY
zx*ZygRQPK@{_d%jSIQ2hhGC1_S2|D2-i>{_$}9F;QGa$X^z#Dn;E1zUIg!hrG&71i
zEVdlJHqq{GN$v;@)z!xyI2P_RN!(obcNXt&hY0HSt+xuwJ+ptDj`xhwc`5tX=Y7Lc
zA^r%u$(g7rxIw~5l(p+lWxJ3;#A1ijffMx~5HhC+dPNT`Y#kAlOxW&|a`>@cC?I}e
zYx$~ich6GhUjGjT>3jVarw$WOjeHRGkRH7b^Yii&nKFs2mv=b$(_Uv-=JRooE)_Q-
zl0}?(;%a4VxqQ`1x_|qzy|zWxVlT3X{DCI!Mrak#jyPKAXiQG)%{=ze`H+L`8Ioca
zKCFJ}y09Hrn~@kCNOKZD{X~AKLiN<txHvCib>D>XHd7wGH=svIalgV*5)Cf#OWQ&0
zzLzsiOiYoIjv7c4LWlLvx(wqn-J(`|`?o%39`}x1vXt^#rYYki)`A#ueK(^WBtD@E
z$MI9^=979MJxg@rDB6!EclPsssE9DR8HorWwjg&&;+?7DgR6dAnNao;Zm&@9V}ZRk
zGzo^VuT83|*wc4@44K%XcC21^%<TAHzi=}_@#yV+4^6-12<4+V-qmdof2s>y#D0B>
zvFm)%lr`K-mylx?=MS+uaHeR9e)G<%dDOwhSf4??U)Qz=MMX!21P|_zb<XH}ARdqF
zJ&F`!B>s&2fY3!|Q#h~$CkmA(KbySx7XIta(jZ*yI7me-efAv|KgE<8Hn7U^L5d3(
zucRANvNRY{Q=bbbMf>xXtg@n?L_0`F{z`E=5X*&3o9dlz!$vEJQtkqLv%4Q4<DEzl
zxQL<#WG^3z3lVjAmOLzVEEq&e_W8@tNL)!aW0X89Q_7X7nP^Kd!I`N9?Db@N9Y1(1
z213V|e-chXrb4504CPRipPND`03yD~1OQkZ8>+C{n`smSA-=^8=_;CI^>6xM9^Xkp
znUk`odus&6X$ZYFZ9#1cWY>LE-rgtr#l+RCQYW~GFJopoo>~1euSU#y0IB~U2nmp<
z*2v>1NVRLVQoJ5qD)Ownawr89^2dP+t&u3(U;>0Mp-&#8ovvyhD))nl+gl%{9lVvI
zw~a#Rb>irIe@`5nfYj8Q8X#w_;7&F}QKr=ZOpyf2lVQeOvm9I+1a^M}zW?)7j=Cii
z-(GF0qESObD027;4M_md3?WbaJ{mIzO}A)8=acJfEarzz-!1Xkt1OGR<1J7YT+o)|
zi}Ppq$$Nj`cUfE(XHO^@VdMcA;8%F4fT=<EWP#i*NW~%`Xu#q3j7brxGPHKs@{wkt
zLu6ZK)Asnf2#`%`N^!U}g1aR2@I;^Zdi$V;xmjlPrB67(hP3ZjzKHv*tJoM1%>haB
zOr*@B&{qq%a!?I{weDy0ngf$Efn<`YSNIA|IEwneX(2-p?w_bvKc4U6d9wxl;4k3d
zOv`Yqh6-cxBvJPoekcEwYc~><_n4Y>raZbPevxCb#}ty`gz_zrULQaNCs6HXKh@tb
zY&>wRCBu#Y0D{%8ZEWox*^{7R4b^IHPBdG`vi@4(rQtpLHIq(d(;_L%6lG`mbO61`
zr=+)`wq`uTF8C~(3Q(cOlKHOH%c6BQa0+ri*-^AaWCaMbcwpw<8n@h%n_f%WC6Bt#
zbGvcuFLmV_TuSKI;^qdNh7VsD*>TpqWruC)H#$FkJ14IBiWu)^IOLd@WBZj_VLpxU
zp#AfYE-8rDPp*X?d;4tW{nw7`C*Y?K3XtcF4cb$u2S{RfvCcP3I?7fXeX<3Py*~pL
z8{cFo2MRpr8h;ZlTyIn_>V5I`PU&CcTLZBMOJ29{>5Em2SVSite&)(;SAO)o=`<uP
z!|}KRe<5B&-kF*rO5E<n(tG)pr^xpHeI3u>r_}c4FG4Ptb}ruHOS+6pw<h<A)kF@e
z|Na?calL$L@PSlUXHafp5uSxYVvjdN&2`0v9c&`bf$C&eR3G1pUK5p1+P4C5$AJJ9
z`NZX#n!zDZR{SQ$IT9+Tcu4jE(aLbbCnp-Xib|)P;wk^)OZQ+e3s-NYsU3~tGmBij
zyqbPG8abY^h0*8N{Y=%WFv~iTMS@5ZZmOQ)Fg@}>{Gc<^kQ<d5{X#(|lLI?vw3L=$
z+`o%A!k~P1LBoMXzOy-}!jbBkmFrJMr6biLC;}B=e-UcFHJ~um`!>o%O5$b$j3p1r
zuzwscl9#6l%Mi*{lkNv0HZh5@H(noj**AU1p%#?E(NH!7sKac8nxkZ<J2geDEdj1_
zi9=JdfbeH*4-yon+*A!Iz;BVDNI2T5Xo5%*CZVxSZL;E~mxu$YPuj|b;R5t@7umqn
zbKb%BpcI4{>m+3|M8Jt^E_!8FSDE*)IZaU!!e>kF6Rz0WIw11>)mpJ-!q1k_+vdyY
zU{0Dc^JW0qBn3>Vw(wGS;X7P?R=h1QU1b20F@FXsQh<lcwdON)!hJ`{@$c1RuK27}
zl!|K3nxSC%N8MwV>t%~A6$U<Jyz6woC?1#CcsI=3dV=GlAlLlja#)7uH-b>3bK_!e
z1=A9jiaXu)cQAM_^xjJVZqCtvJ}B!YSyzp`MX@|Gj>)q0za(}m7*4F1%93$b=zlgz
z4E{UJTb2XBwG*sgs6PBO)>Pe_g@T`WJ)dDoP!Os?@sxzkvf@S-5p}QMp9_n4VbvoC
zCmPb3<6$q}T<-gV)lc~?Z!<SHMFmMqU&i*u8cb~SZj=h?o{#0-UvE5Kiaua$VbSl;
ze=A-7fI~n{W+7S5_<P&DYV3RB`N>7hHuby;7n(*&{4zs5_br)-qv=xPg{G)Ami)$%
zXK{3>9uE5xiRE!H?*VMe6nfMdHnNC*!^P+U=Y$ccA`VqNm_LVlK?nhASEF^cPt}t6
zXjD)w5^A0R#VLc6i`Xq3Hl+r$MZ|96V3-o<Z}9AGrt#rX+9Sz!R;dQfPzyW_{WXc$
zjMj|;4I}A>T&P(BREa}3XVdf|(I2R3lT>sZ3idE49j%|Xry6RAL#2`UQ&gB~B$P21
z%1gFM<05_hRfLqbn0d2wa}`zun`T*!nWTWK09`0By%5DZT@Srtm{EYrh$v$at5b8z
zm^blsX*_Ko4$Q1)w6PhNN9fnd8K>Eq8S5!6)*=@8tm9Uh9c7Hn;4H26j81&k&Do4%
zHjU&gGJv9uk~4?bGb)4AM+~zjf-@Dl^k+6;tF_3J;H+mV^rykuz9`x=JYBIFK;v>h
z1Vc4AP~~PoaVs0G4>sE5jBV#mZ|Bb8^8lkfrDh-lPg|?cQ^bMhB$_TQ8y=D~laN|v
zlP~l&M-iZJ;(fG8Xq;p|j-D|`E~rY#SK;$f;sATN`RGnoBD)rv-mGgJg*FFZ2Nyvd
z5~$5B<R8tMVdf-_u!cr}xe=%yF<rm8$eIGPiiGJCKujkX2yy=&S^Rwq<JEv3=b$Gj
ze?Us278-4U#ztVU5f~U!a&QauGlH2DU?$Dbhow*w6}|cz{csCCDK7K_hkZza>F0q=
usu$f>#A6OiE-%A~6bQMXb>dj>xabEs?1#lty&CL80xZ?JSWgHC?EfD=g@7^u



r...@golang.org

unread,
Mar 14, 2013, 11:48:41 AM3/14/13
to jeff....@gmail.com, nige...@golang.org, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
There may be something to do here, but I don't believe you've done it.


https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go
File src/pkg/image/decode_test.go (right):

https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go#newcode35
src/pkg/image/decode_test.go:35: {"", "testdata/issue5050.gif", 64 <<
8},
the "" thing is ugly. plus you are looking for a specific error. please
just make this one file a special Test that checks exactly what you're
looking for.

https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go
File src/pkg/image/gif/reader.go (right):

https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go#newcode183
src/pkg/image/gif/reader.go:183: if pix, err = ioutil.ReadAll(lzwr); err
!= nil {
you're still reading in the whole file. i guess that's unavoidable, but
it means the CL description misled me a bit. change it to something
like "after reading the file, if the size does not match the bounds,
don't allocate an Image".

in any case, i don't really see the difference here. instead of reading
into m.pix, you're reading into a local variable. what have you saved?
in fact, isn't it worse? in the original, you read no more than the
bounds; here you decode the whole file.

https://codereview.appspot.com/7486051/

Brad Fitzpatrick

unread,
Mar 14, 2013, 12:36:58 PM3/14/13
to jeff....@gmail.com, nige...@golang.org, r...@golang.org, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
The problem was that you could make a GIF file that said:

[ header ] [ image is 99999999 x 99999999 pixels! ] [ lie. only 400 pixels ] [ EOF ]

And we'd read it in that order, and allocate 99999999 * 99999999 pixels ahead of time, only to fail later when we found it was short.

With his change, he reads the dimensions, notes it, and then slurps all the pixels and verifies they match, having only allocated 400 pixels worth of memory.




--

---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.
For more options, visit https://groups.google.com/groups/opt_out.



Michael Jones

unread,
Mar 14, 2013, 1:06:09 PM3/14/13
to Brad Fitzpatrick, jeff....@gmail.com, Nigel Tao, Rob 'Commander' Pike, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
precisely... the "GIF of death"

much larger than:

You might want to think of it like file reading, where you have "read the whole thing" ("by file") or a more timid approach, not a buffer at a time like the io library, but with a pre-defined limit.


--
 
---
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+...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.
 
 



--
Michael T. Jones | Chief Technology Advocate  | m...@google.com |  +1 650-335-5765

jeff....@gmail.com

unread,
Mar 14, 2013, 1:22:48 PM3/14/13
to nige...@golang.org, r...@golang.org, brad...@golang.org, m...@google.com, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com

r...@golang.org

unread,
Mar 14, 2013, 1:44:20 PM3/14/13
to jeff....@gmail.com, nige...@golang.org, brad...@golang.org, m...@google.com, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
i had the problem backwards. looks good but please fix these two nits.


https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/decode_test.go
File src/pkg/image/decode_test.go (right):

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/decode_test.go#newcode125
src/pkg/image/decode_test.go:125: t.Errorf("Got error %v", err)
t.Errorf("expected 'not enough pixels' error; got %v", err)

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go
File src/pkg/image/gif/reader.go (right):

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go#newcode189
src/pkg/image/gif/reader.go:189: return fmt.Errorf("gif: not enough
pixels")
return errors.New("gif: not enough pixels")

https://codereview.appspot.com/7486051/

r...@golang.org

unread,
Mar 14, 2013, 1:45:40 PM3/14/13
to jeff....@gmail.com, nige...@golang.org, brad...@golang.org, m...@google.com, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go
File src/pkg/image/gif/reader.go (right):

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go#newcode189
src/pkg/image/gif/reader.go:189: return fmt.Errorf("gif: not enough
pixels")
actually this isn't a very good message. how about
return errors.New("gif: file too short for rectangle defined in header")

https://codereview.appspot.com/7486051/

jeff....@gmail.com

unread,
Mar 15, 2013, 4:30:37 AM3/15/13
to nige...@golang.org, r...@golang.org, brad...@golang.org, m...@google.com, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com

minu...@gmail.com

unread,
Mar 15, 2013, 4:37:59 AM3/15/13
to jeff....@gmail.com, nige...@golang.org, r...@golang.org, brad...@golang.org, m...@google.com, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
could we use a smaller gif for the test?
i think we can just take a minimal gif
and manually corrupt the bounding box
size in its header.

https://codereview.appspot.com/7486051/

nige...@golang.org

unread,
Mar 15, 2013, 4:42:41 AM3/15/13
to jeff....@gmail.com, r...@golang.org, brad...@golang.org, m...@google.com, minu...@gmail.com, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
On 2013/03/14 16:36:59, bradfitz wrote:
> [ header ] [ image is 99999999 x 99999999 pixels! ] [ lie. only 400
pixels
> ] [ EOF ]

> And we'd read it in that order, and allocate 99999999 * 99999999
pixels
> ahead of time, only to fail later when we found it was short.

> With his change, he reads the dimensions, notes it, and then slurps
all the
> pixels and verifies they match, having only allocated 400 pixels worth
of
> memory.

Well, if your program has memory problems with this bad image, would it
also have memory problems with a legit GIF that was actually 99999999 *
99999999? Even if that's a lot of black pixels, a long stream of zeroes
still compresses very well. This CL might be fixing a particular symptom
without addressing a bigger problem of being able to decode untrusted
images without allowing denial of service. That problem isn't specific
to GIF either.

Some more thought is required.

https://codereview.appspot.com/7486051/

Nigel Tao

unread,
Mar 15, 2013, 7:17:09 AM3/15/13
to Jeff R. Allen, Rob 'Commander' Pike, Brad Fitzpatrick, Michael Jones, minux ma, golang-dev, re...@codereview-hr.appspotmail.com
On Fri, Mar 15, 2013 at 7:42 PM, <nige...@golang.org> wrote:
> Some more thought is required.

I dropped some more thoughts at
https://code.google.com/p/go/issues/detail?id=5050#c1

jeff....@gmail.com

unread,
Mar 15, 2013, 12:19:23 PM3/15/13
to nige...@golang.org, r...@golang.org, brad...@golang.org, m...@google.com, minu...@gmail.com, golan...@googlegroups.com, re...@codereview-hr.appspotmail.com
Reply all
Reply to author
Forward
0 new messages