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

x/debug/cmd/viewcore: NPE in readDebugInfo #44757

Open
tbg opened this issue Mar 3, 2021 · 2 comments
Open

x/debug/cmd/viewcore: NPE in readDebugInfo #44757

tbg opened this issue Mar 3, 2021 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@tbg
Copy link

tbg commented Mar 3, 2021 •
edited
Loading

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

$ go version
go version go1.15.5 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/tobias/.cache/go-build"
GOENV="/home/tobias/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/tobias/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/tobias/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/tobias/go/src/github.com/cockroachdb/cockroach/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-build548952027=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Use viewcore as of golang/debug@6a9ae25 on these files

$ viewcore core.1082827 
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x7017e2]
goroutine 1 [running]:
golang.org/x/debug/internal/core.(*Process).readDebugInfo(0xc00020c000, 0xc00019e088, 0x0)
	/home/tobias/go/src/golang.org/x/debug/internal/core/process.go:660 +0x6a2
golang.org/x/debug/internal/core.Core(0x7ffdb23b1232, 0xc, 0x0, 0x0, 0x0, 0x0, 0x20, 0x7f90c5588e98, 0xc000190460)
	/home/tobias/go/src/golang.org/x/debug/internal/core/process.go:161 +0x1f0
main.readCore(0xc0001f7c90, 0x7686c6, 0x828840, 0xc000183080)
	/home/tobias/go/src/golang.org/x/debug/cmd/viewcore/main.go:262 +0xb2
main.runRoot(0xb6b920, 0xba5130, 0x0, 0x0)
	/home/tobias/go/src/golang.org/x/debug/cmd/viewcore/main.go:288 +0x55
github.com/spf13/cobra.(*Command).execute(0xb6b920, 0xc000190030, 0x0, 0x0, 0xb6b920, 0xc000190030)
	/home/tobias/go/src/github.com/spf13/cobra/command.go:859 +0x2c2
github.com/spf13/cobra.(*Command).ExecuteC(0xb6b920, 0x89a4b2, 0x1, 0xc0001f7f00)
	/home/tobias/go/src/github.com/spf13/cobra/command.go:964 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
	/home/tobias/go/src/github.com/spf13/cobra/command.go:900
main.main()
	/home/tobias/go/src/golang.org/x/debug/cmd/viewcore/main.go:244 +0x11f

I instrumented readDebugInfo and basically it was looking up the empty string in the map here and then crashing:

	if exe == nil {
		f := p.files[p.mainExecName]
		fmt.Println(p.mainExecName)
		fmt.Println(f)
		fmt.Println(p.files)
		if f.err != nil {
			p.dwarfErr = f.err
			return nil
		}

It works with this invocation:

viewcore core.kvserver.test --exe=/home/tobias/go/src/github.com/cockroachdb/cockroach/kvserver.test

(I got that string from fmt.Println(p.files) above.

What did you expect to see?

I would've either expected an error asking me to set --exe or, better, it auto-inferring the executable name correctly.

What did you see instead?

An opaque NPE.
@tbg tbg changed the title x/debug/cmd/viewcore: NPE in x/debug/cmd/viewcore: NPE in readDebugInfo Mar 3, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Mar 5, 2021

CC @hyangah, @randall77 via owners.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 5, 2021
@dmitshur dmitshur added this to the Unreleased milestone Mar 5, 2021
@seankhliao seankhliao changed the title x/debug/cmd/viewcore: NPE in readDebugInfo x/debug/cmd/viewcore: NPE in readDebugInfo Jun 18, 2021
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/506558 mentions this issue: internal/core: refactor loading for maintainability

gopherbot pushed a commit to golang/debug that referenced this issue Jun 30, 2023
@prattmic
internal/core: refactor loading for maintainability
180a5d4 
Loading a new core.Process in core.Core is a rather long, complex
process. Currently, this function creates a stub Process with very few
fields set, and then subsequent method calls initially the remaining
fields.

The problem with this approach is that the inputs/requirements and
outputs of these initialization methods is poorly documented. These
methods (e.g., readCore) usually have some prerequisite fields in Process
that must be set on entry, and some fields that they set prior to
return.

Neither of these are well documented, and many are far from obvious. For
example, Process.mainExecName is initialized in readCore -> readNote ->
readNTFile -> openMappedFile.

This is made more of a mess by the fact that Process tries to do most
initialization with a single pass, resulting in fields getting
initialized in unintuitive locations (such as mainExecName).

These combine to make refactoring to support new functionality
difficult, as changing ordering breaks implicit dependencies between
methods.

This CL addresses these issues by eliminating the iterative
initialization of Process. Instead, Process is only instantiated once
all fields are ready. During loading, all work is done by free
functions/types, where inputs and outputs are explicit via function
arguments/returns.

This may inadvertently improve golang/go#44757, as it makes main binary
lookup more explicit.

This CL intends to keep behavior the same, but there are a few changes:

* If the entry point is unknown, We no longer apply the heuristic of
  assuming that first executable mapping is the main binary. This could
  be added back, but it is probably better to ensure that the entry
  point is available on all architectures.

* Failure to ELF parse a file for symbols isn't a hard error (as the
  mapped file might not even be an ELF).

For golang/go#57447.

Change-Id: I7c3712ccb99ceddddc6adbae57d477c25ff4e5ba
Reviewed-on: https://go-review.googlesource.com/c/debug/+/506558
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gabyhelp gabyhelp mentioned this issue Jan 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@dmitshur @tbg @gopherbot