debug/pe: fatal error: runtime: out of memory on NewFile

[mmeloni]

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

$ go version
go version go1.15.7 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/falce/.cache/go-build"
GOENV="/home/falce/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/falce/.gvm/pkgsets/go1.15.7/global/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/falce/.gvm/pkgsets/go1.15.7/global"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/falce/.gvm/gos/go1.15.7"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/falce/.gvm/gos/go1.15.7/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/falce/vchain/vcn/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build610876847=/tmp/go-build -gno-record-gcc-switches"

What did you do?

To reproduce download and unzip 000182.zip

package main

import (
	"debug/pe"
	"log"
	"os"
)


func main() {
	var file, err = os.OpenFile("path-to-the-unzipped/000182.sst", os.O_RDWR, 0644)
	if err!=nil {
		log.Fatal(err)
	}
	defer file.Close()

	_, err = pe.NewFile(file)
	if err!=nil {
		log.Fatal(err)
	}
}

What did you expect to see?

I expected that the method returns a valid pe.FIle

What did you see instead?

runtime stack:
runtime.throw(0x51b52c, 0x16)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/panic.go:1116 +0x72
runtime.sysMap(0xc004000000, 0x1400000000, 0x5dd3f8)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/mem_linux.go:169 +0xc6
runtime.(*mheap).sysAlloc(0x5c3020, 0x1400000000, 0xd400800000, 0xc000200000)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/malloc.go:727 +0x532
runtime.(*mheap).grow(0x5c3020, 0x9ffffb, 0x0)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/mheap.go:1344 +0xa5
runtime.(*mheap).allocSpan(0x5c3020, 0x9ffffb, 0x410100, 0x5dd408, 0x18100)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/mheap.go:1160 +0x665
runtime.(*mheap).alloc.func1()
        /home/usr/.gvm/gos/go1.15.7/src/runtime/mheap.go:907 +0x65
runtime.(*mheap).alloc(0x5c3020, 0x9ffffb, 0x7fe4066c0101, 0x7fe42d000c20)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/mheap.go:901 +0x85
runtime.largeAlloc(0x13ffff4fd4, 0x7ffda0430101, 0x7fe42d000c20)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/malloc.go:1177 +0xa8
runtime.mallocgc.func1()
        /home/usr/.gvm/gos/go1.15.7/src/runtime/malloc.go:1071 +0x46
runtime.systemstack(0x0)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/asm_amd64.s:370 +0x66
runtime.mstart()
        /home/usr/.gvm/gos/go1.15.7/src/runtime/proc.go:1116

goroutine 1 [running]:
runtime.systemstack_switch()
        /home/usr/.gvm/gos/go1.15.7/src/runtime/asm_amd64.s:330 fp=0xc000067888 sp=0xc000067880 pc=0x46caa0
runtime.mallocgc(0x13ffff4fd4, 0x50d8e0, 0x300001, 0x0)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/malloc.go:1070 +0x7e6 fp=0xc000067928 sp=0xc000067888 pc=0x40dde6
runtime.makeslice(0x50d8e0, 0xfffff731, 0xfffff731, 0x10000)
        /home/usr/.gvm/gos/go1.15.7/src/runtime/slice.go:98 +0x6f fp=0xc000067960 sp=0xc000067928 pc=0x45040f
debug/pe.readCOFFSymbols(0xc00019e000, 0x528ea0, 0xc00006a210, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/usr/.gvm/gos/go1.15.7/src/debug/pe/symbol.go:36 +0x2f8 fp=0xc000067a98 sp=0xc000067960 pc=0x4eb378
debug/pe.NewFile(0x528d20, 0xc00000e028, 0x0, 0x0, 0x0)
        /home/usr/.gvm/gos/go1.15.7/src/debug/pe/file.go:103 +0x69f fp=0xc000067e30 sp=0xc000067a98 pc=0x4e5c9f
main.main()
        /home/usr/go/src/pe-file-bug/main.go:17 +0x22d fp=0xc000067f88 sp=0xc000067e30 pc=0x4ee54d
runtime.main()
        /home/usr/.gvm/gos/go1.15.7/src/runtime/proc.go:204 +0x1cf fp=0xc000067fe0 sp=0xc000067f88 pc=0x43c1cf
runtime.goexit()
        /home/usr/.gvm/gos/go1.15.7/src/runtime/asm_amd64.s:1374 +0x1 fp=0xc000067fe8 sp=0xc000067fe0 pc=0x46e6e1

cc @vchaindz

[tc-hib]

Hello,

This does not look like a PE file at all.
So, the best you could expect is a graceful error instead of a panic, such as "not a valid PE file".

Here is a shorter reproducer:

package main

import (
	"bytes"
	"debug/pe"
)

func main() {
	pe.NewFile(bytes.NewReader([]byte{
		0x08: 0x10, 0x00, 0x00, 0x00, 0x71, 0x1C, 0xC7, 0xF1, 0x04,
		0xFF: 0,
	}))
}

[mmeloni]

Thanks @tc-hib, yes a graceful error is what is needed in this case.

[randall77]

Not sure what we could do here. PE files don't have anything resembling a magic number in the header, so it's hard to tell if it really is a PE file. The only thing we can do is check the machine makes sense, which we do.

It's unclear to me whether the MS-DOS stub is optional or not. If people always use it, maybe we can check that.

I guess we could check that the allocation sizes are reasonable. This particular example reports way more symbols than the file could possibly contain, and we dutifully try to allocate storage for them.

[tc-hib]

I don't know why this function seems to accept PE files which would directly start with the file header.
I thought both the DOS stub and the PE signature were mandatory.

The optional header should be read sooner because it starts with a magic number.

I don't know what this function is used for, and I know nothing about system programming, but is it acceptable that a corrupt file may cause a useless allocation of several gigabytes? Shouldn't the function detect that it would read past the end of the file?
Or at least use a temporary bytes.Buffer and io.CopyN to get a chance to meet EOF before reaching that size.

[howjmay]

I guess we could check that the allocation sizes are reasonable. This particular example reports way more symbols than the file could possibly contain, and we dutifully try to allocate storage for them.

But I am wondering how could we get the size of the read bytes array with io.ReaderAt?
If we can get the size of the input byte array, then I think this solution is good

[tc-hib]

@howjmay I guess you can try to read the last byte before actually reading the whole thing.
Or you can mimic io.CopyN, or directly use it:

buf := bytes.Buffer{}
_, err = io.CopyN(&buf, r, COFFSymbolSize*int64(fh.NumberOfSymbols))
if err != nil {
	return nil, fmt.Errorf("fail to read symbol table: %v", err)
}
syms := make([]COFFSymbol, fh.NumberOfSymbols)
err = binary.Read(&buf, binary.LittleEndian, syms)

(disclaimer: I'm a beginner, trying to learn as well as helping)

[howjmay]

@tc-hib I like what you suggested, and I tested well. However, I think maybe we should use LimitReader() and Copy() directly?

[gopherbot]

Change https://golang.org/cl/286113 mentions this issue: debug/pe: fix OOM caused by huge NumberOfSymbols