-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/image/webp: excessive memory consumption #10790
Comments
I am not able to replicate the issue. I'm not sure if I'm setting the
After that, |
Just tried again and the issue reproduces on my machine. My environment is: Are you using a 64-bit OS? |
Yep, I'm using 64-bit OS. Setting lower values for ulimit -v doesn't help. |
OK, probably ulimit does not work on darwin. Modify: to print amount of bytes it allocates and/or panic if it allocates more than 1e8. |
yeah, seems like it. It certainly allocates a lot of memory - |
I looked into it. The first 4 bytes after The file gives |
The image is small and the input is small and that part is not compressed. The code should not allocate gigs of memory in such case. Otherwise it is useless for decoding of real world images. Since there is no way to get the total input size, the code should allocate and read in, say, 1MB; if that does not EOF, allocate 2MB, copy and read in another meg, etc. |
What I wrote on at the end of #10399 more or less applies here, after s/BMP/WEBP/: The structure of the file format is first header, then payload. The image dimensions are in the header, and the decoder allocates a pixel buffer after the header is processed but before the payload is processed, because processing the payload involves writing the pixels to somewhere: the pixel buffer. Decode takes an io.Reader, not an io.ReadSeeker, so at the time it allocates the pixel buffer, it cannot know whether or not there's 'enough input' remaining. Conversely, we may be decoding an image downloaded from a socket, where we cannot know how much input is remaining. So I think this is all WAI. For decoding images in general, with compression, knowing the total file size doesn't necessarily mean that you know whether or not there's enough pixel data. LZW-style length-difference or RLE encoding means that very large uncompressed data can compress very, very well. That the various image libraries should reject an 16x1073741836 early, checking some sort of MaxMem option before allocating a buffer, is issue #5050. Reading 1MB, 2MB, etc. isn't the right fix. That would mean unnecessary re-allocation and copying of large slices as the image was filled in, which I suspect will have a measurable performance impact, as well as complicate the code. Even if you did that, RLE compression can compress uniform-color input really, really well, so all this staggered memory approach might do is make a bunch of smaller allocations on the way to OOM, when the input is small in bytes but large (and otherwise valid) in pixels. Again, this is WAI. I appreciate that you're trying to improve the image packages by fuzzing inputs, but until issue #5050 is fixed (which won't be at least until the tree is open for 1.6), the false positive (like this issue and issue #10399) to true positive ratio is possibly becoming too high. If you want to continue fuzzing, "comment 1" on issue #5050 has an approach to avoid "image too large" false positives: buffer the reader, call DecodeConfig, check the image width and height, and conditionally rewind the buffer and call Decode. |
Ah, I spoke to early, and misread the earlier issue comments. This isn't a pixel buffer, this is a separate buffer. I'll fix this. |
Apologies for mischaracterizing the bug, BTW. |
The issue was fixed in CL 10072, but somehow it was not closed by GopherBot. I think it can be safely closed now. |
The following program allocates 3GB+ while input data is 36 bytes and image size is small.
with ulimit -v 1000000 it crashes as:
The allocation request is for 0xbfef3020 (3220123680) bytes.
on commit c5f9292598396797bde21d35a38a3da25f561f81
The text was updated successfully, but these errors were encountered: