image/gif: decoding gif returns `frame bounds larger than image bounds` error

[montanaflynn]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.8.3 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/montanaflynn/Development/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/w7/gd1mgc9n05q_9hrtt2sd75dw0000gn/T/go-build873139005=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Tried to decode this gif:

package main

import (
    "flag"
    "fmt"
    "image/gif"
    "net/http"
    "os"
)

func exitIfError(err error) {
    if err == nil {
        return
    }

    fmt.Fprintf(os.Stderr, "%v\n", err)
    os.Exit(1)
}

func main() {
    gifURL := flag.String("url", "https://cdn.keycdn.com/img/analytics.gif", "the URL of the GIF")
    flag.Parse()
    res, err := http.Get(*gifURL)
    exitIfError(err)
    _, err = gif.DecodeAll(res.Body)
    _ = res.Body.Close()
    exitIfError(err)
}

What did you expect to see?

The gif decoding not to return an error. The gif opens up in browsers, ffmpeg and photoshop.

When using ffmpeg you see this warning log:

[gif @ 0x7faca2801000] Image too wide by 1, truncating.

What did you see instead?

gif: frame bounds larger than image bounds

I found this link which seems related:

https://groups.google.com/forum/#!topic/golang-codereviews/3zxEsAv4IuU

[odeke-em]

Still present even on Go tip 8aee0b8 so Go1.9 as well

go version devel +8aee0b8 Wed Jun 28 22:59:47 2017 +0000 darwin/amd64

/cc @nigeltao

[mrd0ll4r]

Adding this because I saw a bunch of GIF-related issues pop up lately:

I did a lot of GIF decoding with Go at some point as part of a scraper and image analysis tool. This was with 1.5, iirc. I noticed, too, that many of the GIFs that would render just fine in a browser were rejected by image/gif. Also I encountered a bunch of panics, but never got around to reporting them.

I checked some of the rejected files by hand and found that they are in fact invalid. Browsers seem to be much less strict about decoding them, and ffmpeg as well (as can be seen above). In the end I just skipped the invalid GIFs because I didn't need complete results.

On one hand it'd be nice to have a less-strict mode, so we could actually parse most of the GIFs out there, on the other hand rejecting invalid files seems totally fine.

[hai046]

However, non-standard gif is still a lot，i had to fine-tune its source code to be compatible with many non-standard files currently on the market eg:

if left+width-1 > d.width || top+height-1 > d.height {
        return nil, errors.New("[update]  gif: frame bounds larger than image bounds")
    }

[andybons]

I’d love to know how many gifs out there cause an error with gif.DecodeAll and for what reasons.

Relaxing the spec has security implications. So it’s a matter of determining how many gifs “in the wild” this affects and whether the relaxed rules can be done within a reasonable upper bound of complexity.

[montanaflynn]

@andybons I tested 10,000 random gifs and found 66 returned an error when decoding:

https://gist.github.com/montanaflynn/88b5a69a107aa327ad16143561aff1c6

[andybons]

@montanaflynn awesome! Thanks for doing this. I'm also hoping to grab a larger corpus from within Google since we have a pretty large repository of things from the internet ;), but this is very helpful for initial investigation. Thanks again.

[gopherbot]

Change https://golang.org/cl/119319 mentions this issue: image/gif: fix acceptance of one non-significant byte

[iwdgo]

LZW allows a meaningless 00 byte to occur at the end of the block. Close method is handling the case and comments detail the rational. This byte is not the clear code.

When the byte occurs, the size of the image might be too large by 1 byte in any dimension. Calculations of image bounds are adapted in the proposed fix.

If the byte has a value other than 00, an error will be returned as now. Existing tests required to adapt one error message.

The list of all tests contains 20 cases. If you check the dimensions individually, the case occurs 11 times.

    type testCase struct {
        fileURL  string
        extraByte bool
    }
    s := []string{
        "https://www.arvos.org/content/images/2017/01/gcode_square.gif",
        "http://akns-images.eonline.com/eol_images/Entire_Site/2013822/rs_560x315-130922222048-hughdancy.gif?fit=inside|900:auto&output-quality=100",
        "http://akns-images.eonline.com/eol_images/Entire_Site/201828/rs_480x270-180308133710-Katy.gif?fit=inside|900:auto&output-quality=100",
        "http://cimss.ssec.wisc.edu/goes/blog/wp-content/uploads/2017/07/170710_1702utc_1842utc_modis_sst_anim.gif",
        "http://i.imgur.com/XIJyvMv.gif", // white line displays
        "http://i.perezhilton.com/wp-content/uploads/2013/09/anigif_enhanced-buzz-9717-1379602710-27.gif",
        "http://i.somethingawful.com/u/g0m/goldmine/dogwedding.gif",
        "http://moderndata.plot.ly/wp-content/uploads/2017/02/box_plots.gif",
        "http://moderndata.plot.ly/wp-content/uploads/2017/02/create_a_candle.gif",
        "https://cdn.shopify.com/s/files/1/0130/8502/products/PRA_3.gif?v=1363716556",
        "https://cdn.shopify.com/s/files/1/0268/0841/products/4.gif?v=1458714784",
        "https://cdn.shopify.com/s/files/1/0344/6469/files/n_ears.gif?v=1490847899",
        "https://i0.wp.com/moderndata.plot.ly/wp-content/uploads/2016/11/scroll-heatmap.gif?resize=350%2C200",
        "https://img.buzzfeed.com/buzzfeed-static/static/enhanced/webdr05/2013/9/19/10/anigif_enhanced-buzz-9695-1379602716-4.gif",
        "https://lh3.googleusercontent.com/-4DyfaR5i3xk/VKRRA4bjXHI/AAAAAAAAGwA/_mZ2fmQ7uxQ/w564-h564/ZoomableCoffeeWheel.gif",
        "https://madebymany-v2-prod.s3.amazonaws.com/uploads/image/file/1197/medium_387aff2e41ff1758_giphy.gif",
        "https://static1.squarespace.com/static/5682b4961c12101f97a89544/t/5685760525981d3d911db525/1451587084376/",
        "https://tctechcrunch2011.files.wordpress.com/2015/08/fotokite-phi.gif",
        "https://www.arvos.org/content/images/2017/01/gcode_square.gif",
        "https://www.sbs.com.au/popasia/sites/sbs.com.au.popasia/files/styles/double/public/GDragon_sexy_Perfume_spray.gif?itok=vaJi729u&mtime=1408425351",
    }
    testCases := make([]testCase, len(s))
    var gifURL *string
    var res *http.Response
    var err error
    var fail, success int
    for i, h := range s {
        testCases[i] = testCase{h,true}
    }
    for i, u := range testCases {
        // fmt.Printf("%v \n",u)
        gifURL = flag.String("url"+string(i), u.fileURL, "the URL of the GIF")
        flag.Parse()
        res, err = http.Get(*gifURL)
        if err != nil {
            fail++
            fmt.Sprintln(err)
            continue // picture is unavailable
        }
        _, err = gif.DecodeAll(res.Body)
        _ = res.Body.Close()
        if err != nil {
            fail++
            fmt.Println(i, ": ", u.fileURL, " decoding fails with ", err)
        } else {
            success++
        }
    }
    fmt.Println("GIF with one extra byte read : ", fail, " failures / ", success, " successes")

[nigeltao]

LZW allows a meaningless 00 byte to occur at the end of the block.

I don't think that (meaningless LZW bytes) is why the image/gif package is returning an error. I added a comment on https://golang.org/cl/119319

[iwdgo]

The frame and image bounds are checked when the image descriptor is read. The image data is read afterwards and the close of the data block discards that same byte. The check on the bounds must allow one byte outside the image bounds on only one of the dimension to take that case into account. After reading the data, it is needed to check that the bounds were correct.

Instead of allowing the one byte difference every time, the new version of the fix checks the validity of the bounds afterwards. All images previously rejected are now accepted. The case submitted is now correctly rejected.

[iand]

The Tumblr GIF dataset might be useful for testing: https://github.com/raingo/TGIF-Release