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

numbers.range index out of range error #7269

Closed
anderseknert opened this issue Jan 14, 2025 · 1 comment · Fixed by #7270
Closed

numbers.range index out of range error #7269

anderseknert opened this issue Jan 14, 2025 · 1 comment · Fixed by #7270

Comments

@anderseknert
Copy link
Member

Calling numbers.range with low numbers in the args (<500) now takes an optimized path where interned numbers are used. That code has an issue though, leading to an ugly out of range error under certain conditions.

package lab

bug if numbers.range(0, 10)
opa eval -d lab/bug.rego data.lab.bug
panic: runtime error: index out of range [2] with length 2

goroutine 1 [running]:
github.com/open-policy-agent/opa/v1/topdown.generateCheapRange({0x140004c11c0, 0x2, 0x14d6db778?}, 0x1400037e680)
        github.com/open-policy-agent/opa/v1/topdown/numbers.go:99 +0x30c
github.com/open-policy-agent/opa/v1/topdown.builtinNumbersRange({{0x105eea178, 0x10688c2a0}, {0x105ef1080, 0x1400051aaa0}, {0x105edecc0, 0x1400009c2c0}, 0x140003d04e0, {0x105ee3630, 0x140003b49a0}, 0x1400011d590, ...}, ...)
...

Looking at that code, yes, that's an obvious oversight as we don't have a third (step) argument provided here, and so that index lookup should error out:

stepOp, err := builtins.IntOperand(operands[2].Value, 3)

And if at this point you're saying "surely we have tests that would have caught this?", you're not alone. I haven't dug into it any deeper yet but it seems like this for yet mysterious reasons only happens when the numbers.range call isn't "used" in any way. Meaning that while this crashes:

bug if numbers.range(0, 10)

This doesn't!

works if x := numbers.range(0, 10)

I'll look into both fixing the issue as well as make an attempt at figuring out why that only happens in that particular (and luckily, rare) case. Will also try my best to find out who could have written such flawed code.

28f-3015696128

@anderseknert
Copy link
Member Author

Ah, yes, the assignment version gets rewritten by the compiler so that there is a third operand:

package p

bug = true { numbers.range(0, 10, __local1__); __local0__ = __local1__

So this seems like an issue few are likely to encounter then. Oh well, I'll submit a PR to fix it.

anderseknert added a commit to anderseknert/opa that referenced this issue Jan 14, 2025
anderseknert added a commit to anderseknert/opa that referenced this issue Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant