8000 image/gif: decoding gif returns `frame bounds larger than image bounds` error · Issue #20856 · golang/go · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

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

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

montanaflynn opened this issue Jun 29, 2017 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@montanaflynn
Copy link

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
Copy link
Member

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

@o 8000 deke-em odeke-em added this to the Go1.10 milestone Jun 29, 2017
@mrd0ll4r
Copy link

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
Copy link
hai046 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
Copy link
Member

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.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 13, 2018
@montanaflynn
Copy link
Author

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

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

@andybons
Copy link
Member

@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
Copy link
Contributor

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

@iwdgo
Copy link
Contributor
iwdgo commented Jun 16, 2018 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Contributor
iwdgo 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
Copy link
Contributor
iand commented Sep 24, 2018

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

@iand iand mentioned this issue Sep 24, 2018
Open
@andybons andybons removed their assignment Sep 18, 2019
@drswork drswork mentioned this issue Dec 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
0