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

x/image/webp: excessive memory consumption #10790

Closed
dvyukov opened this issue May 12, 2015 · 11 comments
Closed

x/image/webp: excessive memory consumption #10790

dvyukov opened this issue May 12, 2015 · 11 comments

Comments

@dvyukov
Copy link
Member

dvyukov commented May 12, 2015

The following program allocates 3GB+ while input data is 36 bytes and image size is small.

package main

import (
    "golang.org/x/image/webp"
    "strings"
)

func main() {
    data := "RIFF0000WEBPVP8 00\xef\xbf\xbd\x01\x00\x14\x00\x00\xb24\n\x9d\x01*\x96\x00g\x00"
    cfg, err := webp.DecodeConfig(strings.NewReader(data))
    if err != nil {
        return
    }
    if cfg.Width*cfg.Height > 1e6 {
        return
    }
    if _, err := webp.Decode(strings.NewReader(data)); err != nil {
        return
    }
}

with ulimit -v 1000000 it crashes as:

fatal error: runtime: out of memory

goroutine 1 [running]:
runtime.systemstack_switch()
    src/runtime/asm_amd64.s:216 fp=0xc20803f918 sp=0xc20803f910
runtime.mallocgc(0xbfef3020, 0x4ad900, 0x1, 0xd)
    src/runtime/malloc.go:629 +0x925 fp=0xc20803f9e8 sp=0xc20803f918
runtime.newarray(0x4ad900, 0xbfef3020, 0x0)
    src/runtime/malloc.go:745 +0xcc fp=0xc20803fa28 sp=0xc20803f9e8
runtime.makeslice(0x4a5340, 0xbfef3020, 0xbfef3020, 0x0, 0x0, 0x0)
    src/runtime/slice.go:32 +0x168 fp=0xc20803fa78 sp=0xc20803fa28
golang.org/x/image/vp8.(*Decoder).parseOtherPartitions(0xc20806e000, 0x0, 0x0)
    src/golang.org/x/image/vp8/decode.go:273 +0x5e fp=0xc20803fb70 sp=0xc20803fa78
golang.org/x/image/vp8.(*Decoder).parseOtherHeaders(0xc20806e000, 0x0, 0x0)
    src/golang.org/x/image/vp8/decode.go:311 +0x1f0 fp=0xc20803fc28 sp=0xc20803fb70
golang.org/x/image/vp8.(*Decoder).DecodeFrame(0xc20806e000, 0xd08010600, 0x0, 0x0)
    src/golang.org/x/image/vp8/decode.go:338 +0x4e fp=0xc20803fca8 sp=0xc20803fc28
golang.org/x/image/webp.decode(0x7f541f68b1c0, 0xc208016420, 0x401f00, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    src/golang.org/x/image/webp/decode.go:96 +0x8d5 fp=0xc20803fe58 sp=0xc20803fca8
golang.org/x/image/webp.Decode(0x7f541f68b1c0, 0xc208016420, 0x0, 0x0, 0x0, 0x0)
    src/golang.org/x/image/webp/decode.go:259 +0x55 fp=0xc20803feb8 sp=0xc20803fe58
main.main()
    webp.go:19 +0x233 fp=0xc20803ff90 sp=0xc20803feb8

The allocation request is for 0xbfef3020 (3220123680) bytes.

on commit c5f9292598396797bde21d35a38a3da25f561f81

@syst3mw0rm
Copy link
Contributor

I am not able to replicate the issue. I'm not sure if I'm setting the ulimit correctly though. I run ulimit -v 1000000 as mentioned. ulimit -a output:

-t: cpu time (seconds)              unlimited
-f: file size (blocks)              unlimited
-d: data seg size (kbytes)          unlimited
-s: stack size (kbytes)             8192
-c: core file size (blocks)         0
-v: address space (kbytes)          976
-l: locked-in-memory size (kbytes)  unlimited
-u: processes                       709
-n: file descriptors                2560

After that, go run webp.go seems to work fine. Am I doing anything wrong?

@dvyukov
Copy link
Member Author

dvyukov commented May 13, 2015

Just tried again and the issue reproduces on my machine.

My environment is:
go version devel +71bf182 Tue May 12 04:02:25 2015 +0000 linux/amd64
x/image is on commit c5f9292598396797bde21d35a38a3da25f561f81
$ ulimit -v
1000000

Are you using a 64-bit OS?
I am not sure why ulimit -v says 976 on your machine. You asked for 10000000, and 976 kb is not enough even to start a Go program. Anyway, try to set ulimit -v to a smaller value.

@syst3mw0rm
Copy link
Contributor

go version devel +71bf182 Tue May 12 04:02:25 2015 +0000 darwin/amd64
x/image is pointing to master - c5f9292598396797bde21d35a38a3da25f561f81

Yep, I'm using 64-bit OS.
Darwin Aamirs-MacBook-Pro.local 14.3.0 Darwin Kernel Version 14.3.0: Mon Mar 23 11:59:05 PDT 2015; root:xnu-2782.20.48~5/RELEASE_X86_64 x86_64 i386 MacBookPro11,1 Darwin

Setting lower values for ulimit -v doesn't help.

@dvyukov
Copy link
Member Author

dvyukov commented May 13, 2015

OK, probably ulimit does not work on darwin. Modify:
golang.org/x/image/vp8.(*Decoder).parseOtherPartitions(0xc20806e000, 0x0, 0x0)
src/golang.org/x/image/vp8/decode.go:273 +0x5e fp=0xc20803fb70 sp=0xc20803fa78

to print amount of bytes it allocates and/or panic if it allocates more than 1e8.

@syst3mw0rm
Copy link
Contributor

yeah, seems like it. It certainly allocates a lot of memory - 3220123680

@syst3mw0rm
Copy link
Contributor

I looked into it. The first 4 bytes after VP8 is chunkLen. In this case, it is [48 48 239 191] in little-endian form which actually translates to a huge number - 3220123696.

The file gives EOF error when decoding anyway. I'd just say it is a corrupt file and behaviour is expected.

@dvyukov
Copy link
Member Author

dvyukov commented May 13, 2015

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.

@nigeltao
Copy link
Contributor

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.

@nigeltao
Copy link
Contributor

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.

@nigeltao nigeltao reopened this May 14, 2015
@nigeltao
Copy link
Contributor

Apologies for mischaracterizing the bug, BTW.

@changkun
Copy link
Member

The issue was fixed in CL 10072, but somehow it was not closed by GopherBot. I think it can be safely closed now.

@golang golang locked and limited conversation to collaborators Jan 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants