cuelang / cue

CUE has moved to https://github.com/cue-lang/cue
https://cuelang.org
Apache License 2.0
3.08k stars 171 forks source link

Panic during cue eval #598

Closed verdverm closed 3 years ago

verdverm commented 3 years ago

What version of CUE are you using (cue version)?

master @ 6c49cf0

the previous commit(s) do not have this panic

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

goroutine 1 [running]:
cuelang.org/go/cmd/cue/cmd.recoverError(0xc000723ec0)
        /home/tony/cue/cue/cmd/cue/cmd/root.go:221 +0x95
panic(0xbcfac0, 0x11a5f00)
        /usr/local/go/src/runtime/panic.go:969 +0x1b9
cuelang.org/go/internal/core/adt.(*Vertex).Default(0x0, 0xd9e0e0)
        /home/tony/cue/cue/internal/core/adt/default.go:46 +0x37
cuelang.org/go/internal/core/adt.(*OpContext).node(0xc0002cd6c0, 0xd99420, 0xc0003d0cd0, 0x40bc01, 0xc000020000)
        /home/tony/cue/cue/internal/core/adt/context.go:676 +0x1cc
cuelang.org/go/internal/core/adt.(*SelectorExpr).resolve(0xc00044f0e0, 0xc0002cd6c0, 0xc000390c30)
        /home/tony/cue/cue/internal/core/adt/expr.go:588 +0x54
cuelang.org/go/internal/core/adt.(*OpContext).Resolve(0xc0002cd6c0, 0xc0001df9f0, 0xd93660, 0xc00044f0e0, 0xc00044f0e0, 0x1)
        /home/tony/cue/cue/internal/core/adt/context.go:331 +0xdb
cuelang.org/go/internal/core/eval.(*nodeContext).evalExpr(0xc0000cf980, 0xc0001df9f0, 0xd8bc40, 0xc00044f100, 0x0)
        /home/tony/cue/cue/internal/core/eval/eval.go:1144 +0x49e
cuelang.org/go/internal/core/eval.(*nodeContext).addExprConjunct(0xc0000cf980, 0xc0001df9f0, 0xd8bc40, 0xc00044f100, 0x7ea800000000)
        /home/tony/cue/cue/internal/core/eval/eval.go:1114 +0xaa
cuelang.org/go/internal/core/eval.(*Evaluator).evalVertex(0xc000345e60, 0xc0002cd6c0, 0xc000340360, 0x4, 0x0, 0x0, 0x0)
        /home/tony/cue/cue/internal/core/eval/eval.go:408 +0x2d8
cuelang.org/go/internal/core/eval.(*Evaluator).UnifyAccept(0xc000345e60, 0xc0002cd6c0, 0xc000340360, 0x719804, 0x0, 0x0)
        /home/tony/cue/cue/internal/core/eval/eval.go:283 +0xa7
cuelang.org/go/internal/core/eval.(*Evaluator).Unify(0xc000345e60, 0xc0002cd6c0, 0xc000340360, 0xc0001d4a04)
        /home/tony/cue/cue/internal/core/eval/eval.go:257 +0x50
cuelang.org/go/internal/core/eval.(*nodeContext).completeArcs(0xc0000cf380, 0xc000130004)
        /home/tony/cue/cue/internal/core/eval/eval.go:620 +0xc8
cuelang.org/go/internal/core/eval.(*nodeContext).postDisjunct(0xc0000cf380, 0xbdb004)
        /home/tony/cue/cue/internal/core/eval/eval.go:599 +0x470
cuelang.org/go/internal/core/eval.(*nodeContext).updateResult(0xc0000cf380, 0xc0005b6904, 0x3)
        /home/tony/cue/cue/internal/core/eval/disjunct.go:132 +0x4c
cuelang.org/go/internal/core/eval.(*nodeContext).tryDisjuncts(0xc0000cf380, 0xc000510704, 0xd8bc40)
        /home/tony/cue/cue/internal/core/eval/disjunct.go:203 +0x1a7
cuelang.org/go/internal/core/eval.(*Evaluator).evalVertex(0xc000345e60, 0xc0002cd6c0, 0xc000132240, 0xc000785704, 0x0, 0x0, 0x0)
        /home/tony/cue/cue/internal/core/eval/eval.go:438 +0x3b5
...

fmt.Printf("nil node: %# v\n", v)

...
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.Bottom{Src:(*ast.SelectorExpr)( 0xc000343260), Err:errors.list{(*adt.ValueError)( 0xc000674d20), (*adt.ValueError)( 0xc000674d90)}, Code: 4, HasRecursive:false, ChildError:false, Value:adt.Value(nil)}
nil node: &adt.Bottom{Src:ast.Node(nil), Err:(*errors.posError)( 0xc000094660), Code: 4, HasRecursive:false, ChildError:false, Value:adt.Value(nil)}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.Bottom{Src:(*ast.SelectorExpr)( 0xc000343260), Err:errors.list{(*adt.ValueError)( 0xc000675340), (*adt.ValueError)( 0xc0006753b0)}, Code: 4, HasRecursive:false, ChildError:false, Value:adt.Value(nil)}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.Bottom{Src:(*ast.SelectorExpr)( 0xc000343260), Err:errors.list{(*adt.ValueError)( 0xc0006758f0), (*adt.ValueError)( 0xc000675960)}, Code: 4, HasRecursive:false, ChildError:false, Value:adt.Value(nil)}
nil node: &adt.Bottom{Src:ast.Node(nil), Err:(*errors.posError)( 0xc000094660), Code: 4, HasRecursive:false, ChildError:false, Value:adt.Value(nil)}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.Bottom{Src:(*ast.SelectorExpr)( 0xc000343260), Err:errors.list{(*adt.ValueError)( 0xc000675f10), (*adt.ValueError)( 0xc000675f80)}, Code: 4, HasRecursive:false, ChildError:false, Value:adt.Value(nil)}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.BasicType{Src:(*ast.Ident)(nil), K: 0xff}
nil node: &adt.Bottom{Src:(*ast.SelectorExpr)( 0xc000343260), Err:errors.list{(*adt.ValueError)( 0xc0006804d0), (*adt.ValueError)( 0xc000680540)}, Code: 4, HasRecursive:false, ChildError:false, Value:adt.Value(nil)}
nil node: &adt.StructMarker{NeedClose:false}
verdverm commented 3 years ago

at https://github.com/cuelang/cue/blob/master/internal/core/adt/context.go#L660

adding the following to the empty switch case resolves the issue and does not seem to break my usage / output. Not sure if that is correct or if node should really never be nil at this point...

if node == nil {
    return emptyNode
}
mpvl commented 3 years ago

I'm not entirely clear what you did to produce this panic.

verdverm commented 3 years ago

I cue eval'd one of my mega configs at that commit. https://github.com/hofstadter-io/hofmod-server or _saas repo repro. Haven't found a minimal example yet...

mpvl commented 3 years ago

Could you try it with tip again? There have been some significant changes in the code so the stack trace won't say much anymore and the problem may have been fixed.

Thanks!

verdverm commented 3 years ago

Sure, was just reading some of them on Gerrit that seem to be the ones your merged. I think I'm actually following those CL's a bit!

mpvl commented 3 years ago

I somehow think these won't fix it though. There is something fishy. The marker should only be obtainable from node, which then cannot be nil. So if that is the case, it must have been returned by getDefault, which should never happen. So I must be missing something. (Or evalState, which is even worse.)

mpvl commented 3 years ago

So would be good where this StructMarker is coming from, assuming it is not node.Value. That is the issue.

Also, are you using it through the API or is this produced by the cue tool?

verdverm commented 3 years ago

produced by both, repro'd with cue exclusively

mpvl commented 3 years ago

Interesting. Looked at the code but will be hard to figure it out unless I know where the StructMarker is coming from. So if you can trace that back that would be great. Basically, a StructMarker should never be returned by any function.

verdverm commented 3 years ago

Something like the check here for the StructMarker case? Is that the condition we want to find the source of? ( I am seeing them both print many times, always together )

(also happy to run through delve and the input with a screen schare, ping me on slack)

func (c *OpContext) node(x Expr, scalar bool) *Vertex {
        v := c.evalState(x, EvaluatingArcs)

        v, ok := c.getDefault(v, scalar)
        if !ok {
                // Error already generated by getDefault.
                return emptyNode
        }

        node, ok := v.(*Vertex)
        if ok {
                v = node.Value
        }
        switch nv := v.(type) {
        case nil:
                c.addErrf(IncompleteError, pos(x), "incomplete value %s", c.Str(x))
                return emptyNode

        case *Bottom:
                c.AddBottom(nv)
                return emptyNode

        case *ListMarker:
                // fmt.Println("case: *ListMarker")
        case *StructMarker:
                if !ok {
                        fmt.Println("case: *StructMarker", )
                        if node == nil {
                                fmt.Println("nil StructMarker", )
                                return emptyNode
                        }
                }

        default:
                if v.Kind()&StructKind != 0 {
                        c.addErrf(IncompleteError, pos(x),
                                "incomplete feed source value %s (type %s)",
                                x.Source(), v.Kind())
                        return emptyNode

                } else if !ok {
                        c.addErrf(0, pos(x), // TODO(error): better message.
                                "invalid operand %s (found %s, want list or struct)",
                                x.Source(), v.Kind())
                        return emptyNode
                }
        }
        return node.Default()
}
verdverm commented 3 years ago

adding a case to getDefault...

func (c *OpContext) getDefault(v Value, scalar bool) (result Value, ok bool) {
        var d *Disjunction
        switch x := v.(type) {
        default:
                return v, true

        // This always prints with the two prints in the last comment
        case *StructMarker:
                fmt.Println("STRUCT")
                return v, true

        case *Vertex:
                // TODO: return vertex if not disjunction.
                switch t := x.Value.(type) {
                case *Disjunction:
                        d = t

                case *StructMarker, *ListMarker:
                        return v, true

                case *Vertex:
                        return c.getDefault(t, scalar)

                default:
                        if !scalar {
                                return v, true
                        }
                        return t, true
                }

        case *Disjunction:
                d = x
        }

        if d.NumDefaults != 1 {
                c.addErrf(IncompleteError, c.pos(),
                        "unresolved disjunction %s (type %s)", c.Str(d), d.Kind())
                return nil, false
        }
        return c.getDefault(d.Values[0], scalar)
}
verdverm commented 3 years ago

This is always printing as well

func (c *OpContext) node(x Expr, scalar bool) *Vertex {
        v := c.evalState(x, EvaluatingArcs)

        switch v.(type) {
                case *StructMarker:
                        fmt.Println("evalState return StructMarker")
        }

        v, ok := c.getDefault(v, scalar)
        if !ok {
                // Error already generated by getDefault.
                return emptyNode
        }
        ....
mpvl commented 3 years ago

Good to know. So evalState is returning a StructMarker when it shouldn't. Will take a look.

mpvl commented 3 years ago

I will have a solution: I'm making the markers different types so that the compiler will enforce correctness.

mpvl commented 3 years ago

See these three CLs: remote: https://cue-review.googlesource.com/c/cue/+/7764 internal/core/adt: add methods for concreteness [NEW] remote: https://cue-review.googlesource.com/c/cue/+/7765 internal/core/adt: introduce base value type [NEW] remote: https://cue-review.googlesource.com/c/cue/+/7766 internal/core/adt: automatic renaming

These make ListMarker and StructMarker a separate type, to the point that functions that return or pass them to the adt.Expr or adt.Value types. There are some casts left, but these are much more localized.

I ran into some funky stuff. Didn't seem too critical, but I can imagine it could have led to your issue. So hopefully it is gone now.

cueckoo commented 3 years ago

This issue has been migrated to https://github.com/cue-lang/cue/issues/598.

For more details about CUE's migration to a new home, please see https://github.com/cue-lang/cue/issues/1078.