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

contains + function rule head causes nil pointer deref #5525

Closed
philipaconrad opened this issue Jan 3, 2023 · 0 comments · Fixed by #5526
Closed

contains + function rule head causes nil pointer deref #5525

philipaconrad opened this issue Jan 3, 2023 · 0 comments · Fixed by #5526
Assignees
Labels

Comments

@philipaconrad
Copy link
Contributor

Short description

  • OPA Version: v0.47.4 and earlier
  • Affects Rego Playground

According to the docs, the contains keyword can only be used as a syntactic sugar for partial set definitions. Unfortunately, the parser is more than happy to accept the keyword in function definitions, which causes a panic down the line in ast/compile later on.

This occurs because the parser (by design!) does not populate the rule.Head.Value field when parsing contains AST nodes, and this breaks how functions are handled later on in ast/compile where that value is assumed to be non-nil.

This panic case can be observed on the Rego Playground as well.

Example:

package test

import future.keywords.contains

p(id) contains x {
	x := id
}

Result:

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=0x0 pc=0x844314]

goroutine 1 [running]:
github.com/open-policy-agent/opa/ast.(*Compiler).compile.func1()
	/opa-devel/ast/compile.go:1364 +0x49
panic({0x10470c0, 0x1d98e50})
	/usr/local/go/src/runtime/panic.go:884 +0x212
github.com/open-policy-agent/opa/ast.(*TypeEnv).Get(0xc0003aa240?, {0x1142a60?, 0x0?})
	/opa-devel/ast/env.go:34 +0x54
github.com/open-policy-agent/opa/ast.(*typeChecker).checkRule(0xc0000a5ce0, 0xc0003aa1c8, 0x1085780?, 0xc0005a3040)
	/opa-devel/ast/check.go:231 +0x80e
github.com/open-policy-agent/opa/ast.(*typeChecker).CheckTypes(0xc0000a5ce0, 0x1?, {0xc0003a6b50, 0x1, 0xc0003688f0?}, 0xc0003688f0?)
	/opa-devel/ast/check.go:142 +0xca
github.com/open-policy-agent/opa/ast.(*Compiler).checkTypes(0xc000391a40)
	/opa-devel/ast/compile.go:1319 +0x22c
github.com/open-policy-agent/opa/ast.(*Compiler).runStage(0x103b800?, {0x11c30e3?, 0x11b43f3?}, 0xe?)
	/opa-devel/ast/compile.go:1349 +0xe5
github.com/open-policy-agent/opa/ast.(*Compiler).compile(0xc000391a40)
	/opa-devel/ast/compile.go:1369 +0xea
github.com/open-policy-agent/opa/ast.(*Compiler).Compile(0xc000391a40, 0xc00038fb00)
	/opa-devel/ast/compile.go:457 +0x3c5
github.com/open-policy-agent/opa/tester.(*Runner).runTests(0xc0001ed680, {0x1593878?, 0xc0005a2780}, {0x15892a0, 0xc00038fe00}, 0x1, 0xc000249d40)
	/opa-devel/tester/runner.go:344 +0x631
github.com/open-policy-agent/opa/tester.(*Runner).RunTests(...)
	/opa-devel/tester/runner.go:264
github.com/open-policy-agent/opa/cmd.runTests({0x1593878, 0xc0005a2780}, {0x15892a0, 0xc00038fe00}, 0xc0001ed680, {0x158bfa0, 0xc0004890b0})
	/opa-devel/cmd/test.go:303 +0x113
github.com/open-policy-agent/opa/cmd.opaTest({0xc000248fe0, 0x1, 0x1})
	/opa-devel/cmd/test.go:285 +0xce5
github.com/open-policy-agent/opa/cmd.glob..func5(0x1db2740?, {0xc000248fe0?, 0x1?, 0x1?})
	/opa-devel/cmd/test.go:147 +0x27
github.com/spf13/cobra.(*Command).execute(0x1db2740, {0xc000248fa0, 0x1, 0x1})
	/opa-devel/vendor/github.com/spf13/cobra/command.go:920 +0x847
github.com/spf13/cobra.(*Command).ExecuteC(0x1db1ea0)
	/opa-devel/vendor/github.com/spf13/cobra/command.go:1044 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
	/opa-devel/vendor/github.com/spf13/cobra/command.go:968
main.main()
	/opa-devel/main.go:14 +0x25

Expected Behavior

  • OPA should provide a helpful error message, instead of crashing.
@philipaconrad philipaconrad self-assigned this Jan 3, 2023
philipaconrad added a commit to philipaconrad/opa that referenced this issue Jan 4, 2023
Previously, we did not detect misuses of the `contains` keyword, which
is only valid as a syntactic sugar for partial set definition rules.

This commit adds some logic to detect when a function rule head has been
paired with the `contains` keyword, which should generally be an error
condition.

Fixes: open-policy-agent#5525

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
srenatus pushed a commit that referenced this issue Jan 5, 2023
Previously, we did not detect misuses of the `contains` keyword, which
is only valid as a syntactic sugar for partial set definition rules.

This commit adds some logic to detect when a function rule head has been
paired with the `contains` keyword, which should generally be an error
condition.

Fixes: #5525

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant