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: unexpected nil pointer dereferences since map dynamic entry changes #31782

Closed
mark-rushakoff opened this issue May 1, 2019 · 14 comments
Closed
Labels

Comments

@mark-rushakoff
Copy link
Contributor

@mark-rushakoff mark-rushakoff commented May 1, 2019

I wasn't sure if this belonged in runtime, cmd/go, or somewhere else -- please retitle as appropriate.

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

$ go version
go version devel +b098c0f467 Wed May 1 15:12:53 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

This does not reproduce with go version go1.12.3 darwin/amd64.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/mr/Library/Caches/go-build"
GOENV="/Users/mr/Library/Preferences/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/mr/go"
GOPROXY="direct"
GOROOT="/Users/mr/gotip/src/github.com/golang/go"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/Users/mr/gotip/src/github.com/golang/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/mr/go/src/github.com/influxdata/flux/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ct/bl4_z3g51ks8239_r2k07v_40000gn/T/go-build385871975=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I ran the tests for github.com/influxdata/flux:

$ cd $GOPATH/src/github.com/influxdata/flux
$ git checkout v0.28.3
$ go test ./stdlib/

What did you expect to see?

Tests passing like with go 1.12.3.

What did you see instead?

A panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x11d5f12]

goroutine 1 [running]:
github.com/influxdata/flux/semantic.function.MonoType(0x0, 0x20be9c8, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x203000, 0x203000, ...)
	/Users/mr/go/src/github.com/influxdata/flux/semantic/poly_types.go:498 +0x42
github.com/influxdata/flux/values.(*function).Type(0xc000181ef0, 0xc000181f20, 0x17c5edb)
	/Users/mr/go/src/github.com/influxdata/flux/values/function.go:43 +0x34
github.com/influxdata/flux/values.(*object).Set(0xc00018a9c0, 0x17c5edb, 0x4, 0x1a56d20, 0xc000181ef0)
	/Users/mr/go/src/github.com/influxdata/flux/values/object.go:105 +0x19c
github.com/influxdata/flux/interpreter.(*Package).Set(...)
	/Users/mr/go/src/github.com/influxdata/flux/interpreter/package.go:59
github.com/influxdata/flux.registerPackageValue(0x17c6f71, 0x6, 0x17c5edb, 0x4, 0x1a56d20, 0xc000181ef0, 0x2091900)
	/Users/mr/go/src/github.com/influxdata/flux/compile.go:204 +0x129
github.com/influxdata/flux.RegisterPackageValue(...)
	/Users/mr/go/src/github.com/influxdata/flux/compile.go:182
github.com/influxdata/flux/stdlib/system.init.1()
	/Users/mr/go/src/github.com/influxdata/flux/stdlib/system/time.go:14 +0x6b
FAIL	github.com/influxdata/flux/stdlib	0.027s

I bisected go to 85387aa as the first build where this panics. Using its parent commit, 70b890c, the tests pass.

Then I noticed #31777, a separate issue filed related to that bisected commit. I tried checking out master of go (b098c0f) and locally applying the change in https://golang.org/cl/174777, but the panic still occurred.

Unfortunately I do not have time right now to come up with a more minimal repro case.

/cc @randall77

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented May 1, 2019

Yes, this is a dup of #31777. I'll close this one, but the test case should be very useful.

@randall77 randall77 closed this May 1, 2019
@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented May 1, 2019

Sounds like the proposed fix to #31777 may not handle this. Reopening until we have confirmed that this is fixed and has a test.

@josharian josharian reopened this May 1, 2019
@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented May 1, 2019

@randall77 @josharian the problem seems to be this line in OMAPLIT order:

as := nod(OAS, nod(OINDEX, n, r.Left), r.Right)

The old code requires a temp, so that mapassign1 can have addressable key, elem:

// Build list of var[c] = expr.
	// Use temporaries so that mapassign1 can have addressable key, elem.
	// TODO(josharian): avoid map key temporaries for mapfast_* assignments with literal keys.
	tmpkey := temp(m.Type.Key())
	tmpelem := temp(m.Type.Elem())

	for _, r := range entries {
		index, elem := r.Left, r.Right

		setlineno(index)
		a := nod(OAS, tmpkey, index)
		a = typecheck(a, ctxStmt)
		a = walkstmt(a)
		init.Append(a)

		setlineno(elem)
		a = nod(OAS, tmpelem, elem)
		a = typecheck(a, ctxStmt)
		a = walkstmt(a)
		init.Append(a)

		setlineno(tmpelem)
		a = nod(OAS, nod(OINDEX, m, tmpkey), tmpelem)
		a = typecheck(a, ctxStmt)
		a = walkstmt(a)
		init.Append(a)
	}

How can we archive the same behavior for order?

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented May 1, 2019

Hmmm, order.go should allocate those temporaries, the OINDEXMAP case in expr.
It should occur when we call o.stmt on the results of nod(OAS, nod(OINDEX, n, r.Left), r.Right).

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 1, 2019

Change https://golang.org/cl/174837 mentions this issue: cmd/compile: fix order fails to initialize map dynamic entry

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented May 1, 2019

Not yet fully minimized, but here's some progress:

package main

import (
	"fmt"

	"github.com/influxdata/flux/semantic"
)

func main() {
        fmt.Println(semantic.NewFunctionPolyType(semantic.FunctionPolySignature{
                Return: semantic.Time,
        }))
}

This used to print () -> time, but at master it currently prints () -> <nil>.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented May 1, 2019

Standalone test file:

package main

import "fmt"

type Nature int

const Time Nature = 0

var natureNames = []string{
	"time",
}

func (n Nature) String() string {
	return natureNames[n]
}

type function struct {
	ret interface{}
}

type FunctionPolySignature struct {
	Required []string
	Return   interface{}
}

func NewFunctionPolyType2(sig FunctionPolySignature) interface{} {
	return function{
		ret: sig.Return,
	}
}

func (f function) String() string {
	return fmt.Sprint(f.ret)
}

func main() {
	fmt.Println(NewFunctionPolyType2(FunctionPolySignature{
		Return: Time,
	}))
}

Used to print time; now prints <nil>.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented May 1, 2019

More minimal:

package main

type one struct {
	i interface{}
}

type two struct {
	i interface{}
	s []string
}

func main() {
	o := one{i: two{i: 42}.i}
	println(o.i.(int))
}

Should print 42, but currently panics.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented May 1, 2019

That's about as far as I think I have time to dig into this at the moment.

@randall77 or @cuonglm , do you mind investigating further?

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented May 1, 2019

@mdempsky /me will take a look shortly.

Sounds like it dues changes in isStaticCompositeLiteral.

@mark-rushakoff

This comment has been minimized.

Copy link
Contributor Author

@mark-rushakoff mark-rushakoff commented May 2, 2019

Just slightly more reduced:

package main

type foo struct {
	i interface{}
	_, _ interface{}
}

func main() {
	z := foo{i: 42}.i
	println(z.(int))
}

Playing with the other fields in foo seems to trigger the bug. A single anonymous interface field doesn't cause it, but two fields do. _ [1]int doesn't cause it, but _ [2]int does. It happens whether the other fields are before or after i.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented May 2, 2019

@mdempsky @mark-rushakoff I'm running dist test before submit the patch.

I think my assumption is right.

The ONAME case in isStaticCompositeLiteral reports the second i: in o := one{i: two{i: 42}.i} as compile-time constant, why it shouldn't.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented May 2, 2019

@mark-rushakoff with newest patch in CL, I can run the test and both examples run without panic.

@mark-rushakoff

This comment has been minimized.

Copy link
Contributor Author

@mark-rushakoff mark-rushakoff commented May 3, 2019

Confirmed that the behavior is fixed on tip. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.