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.

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

x/image/draw: panic resizing image.Uniform #60412

Open
whereswaldon opened this issue May 24, 2023 · 2 comments
Open

x/image/draw: panic resizing image.Uniform #60412

whereswaldon opened this issue May 24, 2023 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@whereswaldon
Copy link
Contributor

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

$ go version
go version go1.20.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/chris/.cache/go-build"
GOENV="/home/chris/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/chris/Code/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/chris/Code/go"
GOPRIVATE="*.playchat.net"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/chris/.local/lib/go-1.20.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/chris/.local/lib/go-1.20.4/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/tmp/repro/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2871775836=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I have code that downloads an image and tries to rescale it. In the event that the download failed, it falls back to a simple image.Uniform and uses that instead. However, this approach actually panics during the rescale in a surprising way:

https://go.dev/play/p/01WIzG_E0nb

panic: runtime error: makeslice: cap out of range

goroutine 1 [running]:
golang.org/x/image/draw.newDistrib(0x4fa1e0, 0x5299f8?, 0x77359400)
        /home/chris/Code/go/pkg/mod/golang.org/x/image@v0.7.0/draw/scale.go:266 +0x265
golang.org/x/image/draw.(*Kernel).newScaler(0x4fa1e0, 0xa, 0xa, 0x77359400, 0x77359400, 0x0)
        /home/chris/Code/go/pkg/mod/golang.org/x/image@v0.7.0/draw/scale.go:142 +0x5e
golang.org/x/image/draw.(*Kernel).Scale(0xc00006a058?, {0x49c700, 0xc00002a040}, {{0x7f602a27cb88?, 0x7f602a271108?}, {0x60?, 0x500b00?}}, {0x49c5c8, 0xc000014230}, {{0xffffffffc4653600, ...}, ...}, ...)
        /home/chris/Code/go/pkg/mod/golang.org/x/image@v0.7.0/draw/scale.go:126 +0xa5
main.main()
        /tmp/repro/main.go:14 +0x165
exit status 2

What did you expect to see?

I expected rescaling an image.Uniform to be trivial since it doesn't actually need to do any work.

What did you see instead?

Because image.Uniform.Bounds() returns an effectively-infinite size, using the bounds of the image as input to the rescale operation makes the rescale try to allocate a buffer larger than available memory.

I understand that I can special-case this in my logic, but it seems desirable for this not to crash in general when an image.Uniform happens to be the input to the rescale operation. Applications like mine may be pulling these images from a variety of sources, and needing this special case is surprising.

I'd be happy to submit a patch that handles this as a special case in the rescale if that's the desired approach.

CC @dominikh @nigeltao @pedroleiterocha @JackMordaunt
@whereswaldon whereswaldon changed the title affected/package: affected/package: x/image/draw May 24, 2023
@whereswaldon whereswaldon changed the title affected/package: x/image/draw x/image/draw: panic resizing image.Uniform May 24, 2023
@gopherbot gopherbot added this to the Unreleased milestone May 24, 2023
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 29, 2023
@jespino
Copy link

jespino commented Jun 9, 2023

After some investigation, some details here: The Uniform image is really big (from: (-1000000000,-1000000000) to: (1000000000,1000000000))

That generate that scaling from a rect (0, 0) to (10, 10) it generate a scale value of 200000000. For each "row/column" it calculates the center with center := (float64(x)+0.5)*scale - 0.5. In this case we have 10 columns, so whenever you reach the column 10 (idx = 9), it is center := (float64(9)+0.5)*200000000 - 0.5) == 1899999999.5, to that we add the halfWidth which is in this case 400000000 because we are using CatmullRom. So the final value is 2299999999.5, what it is bigger than the max int32 value so it overflows. Then when it tries to create a new slice with a negative value as size, it fails.

This can be fixed, for example, changing to uint32 for the source struct i and j values, and the n used in the loop inside the newDistrib function.

Anyway this approach is not great, because scaling that huge Uniform image into something small is going to be a heavy lifting, compare to, for example draw.Copy(dest, image.Point{X: 0, Y: 0}, u, dest.Bounds(), draw.Src, &draw.Options{}).

@whereswaldon
Copy link
Contributor Author

Yeah, image.Uniform always sets itself to be huge, I assume so that you can rely on it covering any given part of another image when doing a Porter-Duff composition.

In this case though, scaling an image.Uniform simply doesn't make sense. With every pixel the same color, the result is trivial to compute without any of the hard math. The algorithm could simply emit an unchanged image.Uniform or one with its bounds set to the specified size.

I reported the bug because it's not terribly hard to invoke this scenario accidentally in general image.Image processing if an image.Uniform happens to be passed in.

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
Unreleased
Development

No branches or pull requests
4 participants
@jespino @whereswaldon @gopherbot @seankhliao