Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
montanaflynn opened this issue Jun 29, 2017 · 11 comments

Comments

@montanaflynn
Copy link

commented Jun 29, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 29, 2017

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

@odeke-em odeke-em added this to the Go1.10 milestone Jun 29, 2017

@mrd0ll4r

This comment has been minimized.

Copy link

commented Jun 30, 2017

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.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@hai046

This comment has been minimized.

Copy link

commented Feb 28, 2018

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 andybons self-assigned this Mar 13, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Author

commented Mar 13, 2018

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

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

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@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

This comment has been minimized.

Copy link

commented Jun 16, 2018

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

@iWdGo

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

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

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 30, 2018

@iWdGo

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.