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

debug/elf: timeout/oom in NewFile #54967

Closed
catenacyber opened this issue Sep 9, 2022 · 9 comments
Closed

debug/elf: timeout/oom in NewFile #54967

catenacyber opened this issue Sep 9, 2022 · 9 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@catenacyber
Copy link
Contributor

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

$ go version
go version go1.17.6 darwin/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="/Users/catena/Library/Caches/go-build"
GOENV="/Users/catena/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/catena/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/catena/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.6"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/catena/go/src/github.com/catenacyber/go/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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/pp/dc1dtf9x2js3v0jx_m010nqr0000gn/T/go-build4237848497=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version go1.17.6 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.17.6
uname -v: Darwin Kernel Version 21.3.0: Wed Jan  5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_X86_64
ProductName:	macOS
ProductVersion:	12.2.1
BuildVersion:	21D62
lldb --version: lldb-1316.0.9.41
Apple Swift version 5.6 (swiftlang-5.6.0.323.62 clang-1316.0.20.8)
gdb --version: GNU gdb (GDB) 9.1

What did you do?

Run https://go.dev/play/p/yNYIA1Rditp?v=gotip

What did you expect to see?

The program finishing and printing Hello

What did you see instead?


Program exited.

Found by https://github.com/catenacyber/ngolo-fuzzing on oss-fuzz
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51098

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 9, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Sep 9, 2022

Is this related to #45599? CC @ianlancetaylor @thanm

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 9, 2022
@mknyszek mknyszek added this to the Backlog milestone Sep 9, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Sep 9, 2022

Note that #45599 was fixed about a month ago on tip. I see that you're running go.dev/play against tip, so maybe this is still an issue?

@catenacyber
Copy link
Contributor Author

Yes this is an issue on tip, so likely similar to #45599 but different...

@gopherbot
Copy link

Change https://go.dev/cl/429601 mentions this issue: debug/elf: avoid using saferio to read data from SHT_NOBITS sections

@ZekeLu
Copy link
Contributor

ZekeLu commented Sep 10, 2022

The test case uncovers another issue: incorrect huge size (3349099659509720927 bytes) of the SHT_NOBITS section. An SHT_NOBITS section may have a nonzero size, but it occupies no space in the file. Since the section size is "conceptual" and the data is provided by a zeroReader, saferio.ReadData does not help in this case. In fact, saferio.ReadData will make it worse because it will eat up the memory one small block by one small block and results in an OOM finally.

It seems that there is not way to tell whether the section size is valid. So https://go.dev/cl/429601 returns the section data by calling make([]byte, s.Size) directly. When the size is huge, it panics with runtime error: makeslice: len out of range fast.

I think a further fix is to set a limit for the size of the SHT_NOBITS section and refuse to read from this section when the size is larger than the limit. But I can not find a documented limit for this section.

@kortschak
Copy link
Contributor

kortschak commented Sep 10, 2022

The docs at https://refspecs.linuxfoundation.org/LSB_2.1.0/LSB-Core-generic/LSB-Core-generic/elftypes.html have

Name Value Description
SHT_NOBITS 0x8 A section of this type occupies no space in the file but otherwise resembles SHT_PROGBITS. Although this section contains no bytes, the sh_offset member contains the conceptual file offset.

I'd suggest that for this case we do

func (s *Section) Data() ([]byte, error) {
    if s.Type == SHT_NOBITS {
        return nil, nil
    }
    return saferio.ReadData(s.Open(), s.Size)
}

This reflects the wording from the docs in that no data is present, but the user can still obtain the sh_offset through the s.Offset field. The user can obviously also obtain the claimed size through s.Size.

@gopherbot
Copy link

Change https://go.dev/cl/430155 mentions this issue: debug/elf: validate shstrndx

@ZekeLu
Copy link
Contributor

ZekeLu commented Sep 11, 2022

@kortschak I like this solution! See also the discussion in #18667.

Since the CL (https://go.dev/cl/375216) that introduced this behavior has been released since go1.18, it seems that it's too late to change the behavior now.

And this solution requires the caller to check the section type. If the caller would like to check the section type, it can avoid calling (*Section).Data at all when the section type is SHT_NOBITS1. It seems that breaking the compatibility does not worth it.

Regarding the caller of (*Section).Data. I found that debug/elf/file.go should not call (*Section).Data when the section type is SHT_NOBITS. The testing elf file has an invalid shstrndx pointing to the SHT_NOBITS section. According to https://refspecs.linuxfoundation.org/elf/gabi4+/ch4.eheader.html, shstrndx should point to the section name string table, which has the type SHT_STRTAB. I have sent another CL (https://go.dev/cl/430155) to validate shstrndx, which will in turn prevent it from calling (*Section).Data at the first place.

Footnotes

  1. here is one such caller: https://github.com/aclements/go-obj/blob/91d9e299b01bb7d48c3ab33b24cdabb3fec63885/obj/elf.go#L420-L454

@ZekeLu
Copy link
Contributor

ZekeLu commented Sep 18, 2022

@aarzilli Can you take a look at https://go.dev/cl/429601 ? The consensus there is to let (*Section).Data return an error when it is an SHT_NOBITS section. Thank you!

gopherbot pushed a commit that referenced this issue Sep 19, 2022
Changes:

1. When e_shstrndx holds the value SHN_UNDEF (0), the file has no section
name string table. In this case, do not try to set section names .
2. e_shstrndx should point to an SHT_STRTAB section. If it does not, returns
an error.

Reference:
https://refspecs.linuxfoundation.org/elf/gabi4+/ch4.eheader.html

Updates #54967.

Change-Id: Ic8f228061d996fd7845dfa630719a1ba12d2bb60
GitHub-Last-Rev: aeb70ca
GitHub-Pull-Request: #55001
Reviewed-on: https://go-review.googlesource.com/c/go/+/430155
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Sep 27, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 27, 2022
@golang golang locked and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants