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

runtime: function textOff returns incorrect value if multiple text sections are present #35207

Closed
laboger opened this issue Oct 28, 2019 · 8 comments
Assignees
Labels
Milestone

Comments

@laboger
Copy link
Contributor

@laboger laboger commented Oct 28, 2019

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

$ go version
go version go1.12.12 linux/ppc64le

Does this issue reproduce with the latest release?

The incorrect code exists in the latest release. The failure will only happen when trying to look up the textOff for an off that is at the end of a text section if there are multiple text sections. Multiple text sections are not common but do happen in Kubernetes and Openshift.

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

go env Output
$ go env
linux/ppc64le RHEL 7.5

What did you do?

Built and ran the tests for Openshift. By default -race is used for testing.

mkdir -p ~/gocode/src/github.com/openshift
cd ~/gocode/src/github.com/openshift
git clone https://github.com/openshift/origin
cd ~/gocode/src/github.com/openshift/origin
export GOCODE=~/gocode

To run all the tests:

GOTEST_FLAGS='-p 8' TIMEOUT=360s TEST_KUBE=true KUBERNETES_SERVICE_HOST= hack/test-go.sh

To run the test that just fails:

go test -p 8 -race -cover -covermode atomic -timeout 360s github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/registry/core/node/storage

What did you expect to see?

No failures.

What did you see instead?

[boger@moose1 origin]$ go test -p 8 -race -cover -covermode atomic -timeout 360s github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/registry/core/node/storage
unexpected fault address 0xfe1e
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x1 addr=0xfe1e pc=0x3fffb7da84fc]

goroutine 1 [running, locked to thread]:
runtime.throw(0x121b0080, 0x5)
	/home/boger/golang/go1.12/go/src/runtime/panic.go:617 +0x5c fp=0xc000596100 sp=0xc0005960c0 pc=0x1003f44c
runtime: unexpected return pc for runtime.sigpanic called from 0x3fffb7da84fc
stack: frame={sp:0xc000596100, fp:0xc000596140} stack=[0xc000578000,0xc000598000)
000000c000596000:  00000000121ae2b3  0000000000000001 
000000c000596010:  0000000000000001  0000000000000000 
000000c000596020:  0000000010040820 <runtime.printstring+112>  00000000146d6520 
000000c000596030:  0000000000000002  000000c0005960f6 
000000c000596040:  0000000000000002  00000000121ae2b3 
000000c000596050:  0000000000000001  000000001003f608 <runtime.fatalthrow+72> 
000000c000596060:  0000000010057f7c <runtime.sigpanic+1036>  0000000000000001 
000000c000596070:  0000000010040128 <runtime.printnl+56>  000000001003f44c <runtime.throw+92> 
000000c000596080:  000000c0005960f6  0000000000000006 
000000c000596090:  00000000121ae2b3  000000c0005960a0 
000000c0005960a0:  000000001006e000 <runtime.fatalthrow.func1+0>  000000c000000180 
000000c0005960b0:  000000001003f44c <runtime.throw+92>  000000c0005960c0 
000000c0005960c0:  0000000010057f9c <runtime.sigpanic+1068>  0000000000000001 
000000c0005960d0:  0000000010057f80 <runtime.sigpanic+1040>  0000000010057f84 <runtime.sigpanic+1044> 
000000c0005960e0:  000000c0005960e8  000000001006df80 <runtime.throw.func1+0> 
000000c0005960f0:  00000000121b0080  0000000000000005 
000000c000596100: <00003fffb7da84fc  0000000012671a80 
000000c000596110:  0000000011ef39a0  000000001226d068 
000000c000596120:  00000000121b0080  0000000000000005 
000000c000596130:  0000000010467ba4 <encoding/json.typeFields+6132>  000000000000fe1e 
000000c000596140: >00003fffb7db2c54  0000000000000000 
000000c000596150:  0000000010467cfc <encoding/json.cachedTypeFields+220>  000000001056a264 <text/template/parse.(*Tree).operand+52> 
000000c000596160:  000000c0005967d0  0000000011ef9c40 
000000c000596170:  000000c0001df558  0000000011ef39a0 
000000c000596180:  000000001226d068  0000000000000002 
000000c000596190:  0000000000000002  0000000012671a80 
000000c0005961a0:  0000000011ef39a0  000000c000378280 
000000c0005961b0:  000000000000000a  0000000000000018 
000000c0005961c0:  0000000000000028  000000c000378208 
000000c0005961d0:  0000000000000001  0000000000000001 
000000c0005961e0:  0000000000000000  0000000010166c00 <regexp/syntax.(*compiler).compile+6640> 
000000c0005961f0:  0000000010165730 <regexp/syntax.(*compiler).compile+1312>  0000000012671a80 
000000c000596200:  0000000000000025  0000000000000043 
000000c000596210:  0000000000000009  0000000000000002 
000000c000596220:  0000000000000001  0000000000000003 
000000c000596230:  0000000000000007  0000000000000002 
runtime.sigpanic()
	/home/boger/golang/go1.12/go/src/runtime/signal_unix.go:397 +0x42c fp=0xc000596140 sp=0xc000596100 pc=0x10057f9c

goroutine 19 [chan receive]:
github.com/openshift/origin/vendor/k8s.io/klog.(*loggingT).flushDaemon(0x13d2bb60)
	/home/boger/gocode/src/github.com/openshift/origin/vendor/k8s.io/klog/klog.go:1018 +0x90
created by github.com/openshift/origin/vendor/k8s.io/klog.init.0
	/home/boger/gocode/src/github.com/openshift/origin/vendor/k8s.io/klog/klog.go:404 +0x84

goroutine 93 [chan receive]:
github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil.(*MergeLogger).outputLoop(0xc000150920)
	/home/boger/gocode/src/github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil/merge_logger.go:174 +0x39c
created by github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil.NewMergeLogger
	/home/boger/gocode/src/github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil/merge_logger.go:92 +0xf8

goroutine 128 [chan receive]:
github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil.(*MergeLogger).outputLoop(0xc000288fa0)
	/home/boger/gocode/src/github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil/merge_logger.go:174 +0x39c
created by github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil.NewMergeLogger
	/home/boger/gocode/src/github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil/merge_logger.go:92 +0xf8

goroutine 126 [chan receive]:
github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil.(*MergeLogger).outputLoop(0xc000288e20)
	/home/boger/gocode/src/github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil/merge_logger.go:174 +0x39c
created by github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil.NewMergeLogger
	/home/boger/gocode/src/github.com/openshift/origin/vendor/github.com/coreos/etcd/pkg/logutil/merge_logger.go:92 +0xf8
FAIL	github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/registry/core/node/storage	0.106s

I have a fix for this. After debugging I found that the problem is in runtime/type.go function textOff where it looks up the text offset from an offset. If the 'off' happens to be the last value in the text section, the wrong value is returned.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 28, 2019

Change https://golang.org/cl/203817 mentions this issue: runtime: fix textOff for multiple text sections

@laboger

This comment has been minimized.

Copy link
Contributor Author

@laboger laboger commented Oct 28, 2019

The segv occurs in encoding/json/decode.go function literalStore when trying to invoke the UnmarshalJSON function. The address of that function is incorrect because in the function indirect this code:

if u, ok := v.Interface().(Unmarshaler); ok {
        return u, nil, reflect.Value{}
}

depends on the tab lookup using textOff and gets an invalid value.

@laboger

This comment has been minimized.

Copy link
Contributor Author

@laboger laboger commented Oct 28, 2019

@gopherbot Please backport to go 1.13 and go 1.12.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 28, 2019

Backport issue(s) opened: #35210 (for 1.12), #35211 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

gopherbot pushed a commit that referenced this issue Oct 28, 2019
If a compilation has multiple text sections, code in
textOff must compare the offset argument against the range
for each text section to determine which one it is in.
The comparison looks like this:

if uintptr(off) >= sectaddr && uintptr(off) <= sectaddr+sectlen

If the off value being compared is equal to sectaddr+sectlen then it
is not within the range of the text section but after it. The
comparison should be just '<'.

Updates #35207

Change-Id: I114633fd734563d38f4e842dd884c6c239f73c95
Reviewed-on: https://go-review.googlesource.com/c/go/+/203817
Run-TryBot: Lynn Boger <laboger@linux.vnet.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@bcmills bcmills added the NeedsFix label Nov 5, 2019
@bcmills bcmills added this to the Go1.14 milestone Nov 5, 2019
@laboger

This comment has been minimized.

Copy link
Contributor Author

@laboger laboger commented Nov 5, 2019

This fix for this is already upstream in CL 203817. Backports are pending.

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Nov 26, 2019

@laboger CL 203817 said "Updates #35207" rather than Fixes. Is there more to do here?

@toothrot

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Dec 3, 2019

@laboger Should this issue be closed? Is it resolved now on tip?

@laboger

This comment has been minimized.

Copy link
Contributor Author

@laboger laboger commented Dec 4, 2019

Yes, resolved on tip.

@laboger laboger closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.