open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.55k stars 1.32k forks source link

OPA 0.29.1 panics when assigning non-existent input to variable inside function #3501

Closed andrehaland closed 3 years ago

andrehaland commented 3 years ago

Expected Behavior

OPA to not panic

Actual Behavior

OPA panics when trying to assign non existent input data to a variable inside a function

Steps to Reproduce the Problem

With the following package

package main

is_input_there(x) {
    r = input.that_is_not_there
}

OPA panics with v0.29.1 and not in v0.28.0:

./opa_v0.29.1 version && ./opa_v0.29.1 eval -b . 'data.main.is_input_there("")'
Version: 0.29.1
Build Commit: 3155647
Build Timestamp: 2021-05-27T20:47:57Z
Build Hostname: Mac-1622145057794.local
Go Version: go1.16.3
WebAssembly: available
panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
github.com/open-policy-agent/opa/topdown.evalFunc.eval(0xc000184000, 0xc0001a78c0, 0x3, 0x4, 0xc0001a7940, 0x3, 0x4, 0xc0001d8a80, 0x5619580, 0xc00000f601)
    /Users/runner/work/opa/opa/topdown/eval.go:1509 +0x5ac
github.com/open-policy-agent/opa/topdown.(*eval).evalCall(0xc000184000, 0xc0001a7940, 0x3, 0x4, 0xc0001d8a80, 0x55acb80, 0x10000000e)
    /Users/runner/work/opa/opa/topdown/eval.go:626 +0x74c
github.com/open-policy-agent/opa/topdown.(*eval).evalStep(0xc000184000, 0xc000119710, 0x4, 0x57b0710)
    /Users/runner/work/opa/opa/topdown/eval.go:303 +0x345
github.com/open-policy-agent/opa/topdown.(*eval).evalExpr(0xc000184000, 0xc000119700, 0x6d40f01, 0xc000119700)
    /Users/runner/work/opa/opa/topdown/eval.go:277 +0xef
github.com/open-policy-agent/opa/topdown.(*eval).eval(0xc000184000, 0xc000119700, 0x5, 0x57b1cd0)
    /Users/runner/work/opa/opa/topdown/eval.go:253 +0x35
github.com/open-policy-agent/opa/topdown.(*eval).Run(0xc000184000, 0xc0001196f0, 0xf, 0x57b4bc8)
    /Users/runner/work/opa/opa/topdown/eval.go:84 +0xd3
github.com/open-policy-agent/opa/topdown.(*Query).Iter(0xc000147558, 0x57b49d0, 0xc000130008, 0xc0001a7b80, 0xc0000d6000, 0xc0001e2270)
    /Users/runner/work/opa/opa/topdown/query.go:439 +0x710
github.com/open-policy-agent/opa/rego.(*Rego).eval(0xc000311180, 0x57b49d0, 0xc000130008, 0xc000274000, 0x0, 0x0, 0x0, 0x0, 0x0)
    /Users/runner/work/opa/opa/rego/rego.go:1887 +0x5a5
github.com/open-policy-agent/opa/rego.PreparedEvalQuery.Eval(0xc000311180, 0xc00007f270, 0x57b49d0, 0xc000130008, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    /Users/runner/work/opa/opa/rego/rego.go:353 +0x19b
github.com/open-policy-agent/opa/cmd.eval(0xc0000abc80, 0x1, 0x3, 0x0, 0xc00007e990, 0x1, 0x1, 0x5f04898, 0x0, 0x0, ...)
    /Users/runner/work/opa/opa/cmd/eval.go:288 +0xaa5
github.com/open-policy-agent/opa/cmd.init.4.func2(0xc0000ed400, 0xc0000abc80, 0x1, 0x3)
    /Users/runner/work/opa/opa/cmd/eval.go:221 +0xcf
github.com/spf13/cobra.(*Command).execute(0xc0000ed400, 0xc0000abc50, 0x3, 0x3, 0xc0000ed400, 0xc0000abc50)
    /Users/runner/work/opa/opa/vendor/github.com/spf13/cobra/command.go:856 +0x2c2
github.com/spf13/cobra.(*Command).ExecuteC(0x5ebcf00, 0xc000000180, 0xc00021ff78, 0x400aca5)
    /Users/runner/work/opa/opa/vendor/github.com/spf13/cobra/command.go:960 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
    /Users/runner/work/opa/opa/vendor/github.com/spf13/cobra/command.go:897
main.main()
    /Users/runner/work/opa/opa/main.go:15 +0x31
./opa_v0.28.0 version && ./opa_v0.28.0 eval -b . 'data.main.is_input_there("")'
Version: 0.28.0
Build Commit: 3fbcd71
Build Timestamp: 2021-04-27T13:51:34Z
Build Hostname: c8a0b3ab05bf
Go Version: go1.15.8
WebAssembly: unavailable
{}

Additional Info

Looking at the source code where Go panics github.com/open-policy-agent/opa/topdown.evalFunc.eval(0xc000184000, 0xc0001a78c0, 0x3, 0x4, 0xc0001a7940, 0x3, 0x4, 0xc0001d8a80, 0x5619580, 0xc00000f601) /Users/runner/work/opa/opa/topdown/eval.go:1509 +0x5ac I see that the memoize feature added a direct lookup on index 0 before check if ir.Empty() https://github.com/open-policy-agent/opa/blob/a57fc72536973f54ea33a3fbf13ef0e5ff417caf/topdown/eval.go#L1509-L1513

I would guess that moving this assignment to after the empty check solves the issue, but I have not tested this yet. I will test this locally and raise a PR if it fixes the issue

srenatus commented 3 years ago

🤦 I should have guessed... https://github.com/open-policy-agent/opa/commit/251dcc58d0e74b49feeb195ea6fb489edd3f8868#r51373293

I'm on it. You're correct with the fix, but I'll add a test somewhere, too.

srenatus commented 3 years ago

@andrehaland would you mind reviewing that? we'll cut a release when it's in.

andrehaland commented 3 years ago

@andrehaland would you mind reviewing that? we'll cut a release when it's in.

Done

srenatus commented 3 years ago

Thank you 😃

srenatus commented 3 years ago

✔️ 0.29.2 is here, fixing this.