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.

Sign up for GitHub

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

Jump to bottom

debug/buildinfo,internal/xcoff: oom (infinite loop ?) #63337

Closed
catenacyber opened this issue Oct 2, 2023 · 8 comments
Closed

debug/buildinfo,internal/xcoff: oom (infinite loop ?) #63337

catenacyber opened this issue Oct 2, 2023 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@catenacyber
Copy link
Contributor

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

$ go version
go version go1.21 linux/amd64

Does this issue reproduce with the latest release?

It happens only on gotip, not on go 1.21

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/root/.go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/root/.go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.21"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/src/ngolo-fuzzing/go.mod"
GOWORK=""
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 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2481516251=/tmp/go-build -gno-record-gcc-switches"

What did you do?

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

What did you expect to see?

The program finishing and printing Hello, without allocating too much
(as happens with go 1.21)

Regression range is given as 5fe3f0a:856cf23a8acfa14756a6e9b82ace76f5604262c9

What did you see instead?

Nothing : Program exited

Found by https://github.com/catenacyber/ngolo-fuzzing with oss-fuzz :
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=62574
@mauri870
Copy link
Member

mauri870 commented Oct 2, 2023

I was able to reproduce this in linux as well as macos. I believe it is related to CL514075, issue #61644.

The initial magic bytes matches conditions for xcoff, the routine that is causing the high memory usage is xcoffExe.ReadData, particulary the line with saferio.ReadDataAt

@mauri870
Copy link
Member

mauri870 commented Oct 2, 2023

@gopherbot add NeedsFix

@gopherbot gopherbot added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 2, 2023
@mauri870
Copy link
Member

mauri870 commented Oct 2, 2023

Same issue as #59817, but there it was fixed for PE.

@mauri870
Copy link
Member

mauri870 commented Oct 2, 2023

I think we are missing some validation when opening an xcoff file, the section size for this example is 3989164744982871388 which causes this append to grow the buffer too large, 10MB at a time. The logic appears to be correct, maybe there is something off with the validation that this a valid xcoff file.

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Oct 2, 2023 •
edited

I think the logic is disconnected here:

go/src/debug/buildinfo/buildinfo.go

Lines 151 to 163 in 340a4f5

// Read segment or section to find the build info blob.
// On some platforms, the blob will be in its own section, and DataStart
// returns the address of that section. On others, it's somewhere in the
// data segment; the linker puts it near the beginning.
// See cmd/link/internal/ld.Link.buildinfo.
dataAddr, dataSize := x.DataStart()
if dataSize == 0 {
return "", "", errNotGoExe
}
data, err := x.ReadData(dataAddr, dataSize)
if err != nil {
return "", "", err
}

It tries to locate data section address by section type (x.DataStart()):

go/src/debug/buildinfo/buildinfo.go

Lines 405 to 410 in 340a4f5

func (x *xcoffExe) DataStart() (uint64, uint64) {
if s := x.f.SectionByType(xcoff.STYP_DATA); s != nil {
return s.VirtualAddress, s.Size
}
return 0, 0
}

but then finds a wrong section based on the virtual address (x.ReadData()):

go/src/debug/buildinfo/buildinfo.go

Lines 392 to 403 in 340a4f5

func (x *xcoffExe) ReadData(addr, size uint64) ([]byte, error) {
for _, sect := range x.f.Sections {
if sect.VirtualAddress <= addr && addr <= sect.VirtualAddress+sect.Size-1 {
n := sect.VirtualAddress + sect.Size - addr
if n > size {
n = size
}
return saferio.ReadDataAt(sect, n, int64(addr-sect.VirtualAddress))
}
}
return nil, errors.New("address not mapped")
}

The wrong section happens to be a .bss section that uses zeroReaderAt that reads infinite amount of zeroes:

go/src/internal/xcoff/file.go

Lines 263 to 265 in 340a4f5

if scnptr == 0 { // .bss must have all 0s
r2 = zeroReaderAt{}
}

Its not clear whether sections virtual addresses could actually overlap, if not then this should be validated during parsing.
Also using a never-ending reader does not seem like a safe choice.

Update: apparently buildInfoMagic blob may reference addresses in other sections.

@AlexanderYastrebov
Copy link
Contributor

Maybe using eofReaderAt that always returns 0, io.EOF for .bss instead of zeroReaderAt would be enough to fix this.

@bcmills bcmills changed the title debug/buildinfo: oom (infinite loop ?) debug/buildinfo,internal/xcoff: oom (infinite loop ?) Oct 13, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 13, 2023
@mknyszek mknyszek added this to the Backlog milestone Oct 25, 2023
@mauri870
Copy link
Member

@AlexanderYastrebov Would you be willing to send in a fix? Thanks

@callthingsoff callthingsoff mentioned this issue Jan 4, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/553616 mentions this issue: internal/xcoff: change zeroReaderAt to eofReaderAt for .bss with all 0s

@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Feb 12, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@callthingsoff @ezz-no
internal/xcoff: change zeroReaderAt to nobitsSectionReader for .bss w…
b2390b4 
…ith all 0s

Fixes golang#63337

Change-Id: I239315047e6e4325e2f471108fd764f8dbb7d5b2
GitHub-Last-Rev: cacdf0a
GitHub-Pull-Request: golang#64952
Reviewed-on: https://go-review.googlesource.com/c/go/+/553616
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

Successfully merging a pull request may close this issue.

internal/xcoff: change zeroReaderAt to nobitsSectionReader for .bss with all 0s callthingsoff/go
6 participants
@AlexanderYastrebov @mknyszek @dmitshur @gopherbot @mauri870 @catenacyber