-
Notifications
You must be signed in to change notification settings - Fork 18k
image/png: fatal error: runtime: out of memory
via (*decoder).parseIDAT
#53778
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
Comments
Your program says it exits without error, but it doesn't. My version, which checks the error returned by Decode, reports
https://go.dev/play/p/bAa9vbuJfgT It may be running out of memory or CPU time on the playground, but the input is invalid and the error is reported on a proper machine. Did you find this example by fuzzing? |
I am able to reproduce the reported behavior on Go at HEAD.
|
fatal error: runtime: out of memory
in NewNRGBA64
fatal error: runtime: out of memory
in NewNRGBA64
fatal error: runtime: out of memory
via (*decoder).parseIDAT
(CC @nigeltao) |
Interesting. I am on an M1 with 128GB of memory and see an error, not a crash. |
I ran Rob's version and Ctrl-'ed it after a few seconds and it looks like it is allocating 3.4 TB. Probably different systems handle that differently. |
png.DecodeConfig runs fine and says 255x1677721600, so this may be another instance where untrusted inputs should be checked with DecodeConfig before being passed to Decode. |
I tried it on a smaller M1 and still saw the error, not the crash. My summary: this is a buggy program that does not protect against bad inputs as instructed, but the variant behavior on different CPU types and/or operating systems is worth investigating. |
Yeah, I found it by go-fuzz, and the program exceeded the memory limit of the system when applying for a slice according to mutated size of the image. In fact, what I want to say at the last |
I'd agree with that. I'd also say that the bug is a dupe of #5050 (whose title says |
One approach we could use for this kind of problem is a incremental image build, as in https://go.dev/cl/417477. @nigeltao Any opinions about that approach? Thanks. |
Change https://go.dev/cl/417477 mentions this issue: |
I dropped a code review comment on that page. |
Bringing the discussion back to the issue. It's a fair point that the CL doesn't protect well against zlib. Although I do think of this kind of change more as aimed at a corrupt file than as a defense against an attacker. I also think it's mildly unfortunate to expect people to check the Certainly the CL needs more work, it's just a sketch. What concerns me about these issues is the unrecoverable crash that occurs on many systems, particularly when running in a container. It's not a friendly response to corrupt data. This is related to the discussion at #20169, which remains unresolved. I've been trying to tackle a subset of these issues in the CLs starting at https://go.dev/cl/408678. Basically I'm trying to work within our existing constraints to return an error rather than crashing. This CL, https://go.dev/cl/417477, is an attempt at taking this approach to image/png, but it may be a poor fit. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I attempt to decode an png data, and the details can be seen at:
go.dev/play
What did you expect to see?
It decodes the data successfully or just returns an error
What did you see instead?
It triggered a fatal error like:
go env
OutputFound by go-fuzz,
The text was updated successfully, but these errors were encountered: