Go: runtime: unexpected nil pointer dereferences since map dynamic entry changes

[mark-rushakoff]

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 85387aa364f5ebb8c1c10ee2de78ae8ded5a80dd as the first build where this panics. Using its parent commit, 70b890ce3b72fdab528ee1f93b1950173e9a0992, the tests pass.

Then I noticed https://github.com/golang/go/issues/31777, a separate issue filed related to that bisected commit. I tried checking out master of go (b098c0f467e5ce70b936381c439a0cbafc3316d3) 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]

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

[josharian]

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

[cuonglm]

@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]

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]

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

[mdempsky]

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]

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]

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]

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]

@mdempsky /me will take a look shortly.

Sounds like it dues changes in isStaticCompositeLiteral.

[mark-rushakoff]

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]

@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]

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

[mark-rushakoff]

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