object literal defined using := panics with "corrupt object" #1125
Labels
Comments
srenatus
changed the title
tsandall
added a commit
to tsandall/opa
that referenced
this issue
Dec 21, 2018
During the assignment rewriting stages, variables that have been
assigned locally are rewritten, e.g., k := "foo"; {k: 1} becomes
__local0__ = "foo"; {__local0__: 1}. Previously, the rewriting would
simply mutate all of the term values without considering where the term
was defined. The problem with this was that it would corrupt the
object's hashtable if the rewritten term was an object key.
This commit resolves the issue by making a copy of the object key before
mutating the term value. This approach requires the compiler to copy the
object as well. If this proves to be a performance bottleneck, we can do
something more clever.
Note, it's arguable that the Object struct should not allow keys to be
mutated at all. This would be tricky given the current Object interface.
Perhaps we can improve this in the future to avoid similar issues.
These changes also resolve an issue with the rewritten var set returned
by the query compiler. Previously, all seen vars were returned in the
set (as opposed to only those that had been redeclared.) When the query
compiler inverts the map before returning it could accidentally elide
vars, e.g., given {a: b, b: b} if it inverted this to {b: a} then the
output would be correct but if it inverted this to {b: b} (which it
could depending on the iteration order) the output would be incorrect.
Fixes open-policy-agent#1125
Signed-off-by: Torin Sandall <[email protected]>
tsandall
added a commit
that referenced
this issue
Dec 23, 2018
During the assignment rewriting stages, variables that have been
assigned locally are rewritten, e.g., k := "foo"; {k: 1} becomes
__local0__ = "foo"; {__local0__: 1}. Previously, the rewriting would
simply mutate all of the term values without considering where the term
was defined. The problem with this was that it would corrupt the
object's hashtable if the rewritten term was an object key.
This commit resolves the issue by making a copy of the object key before
mutating the term value. This approach requires the compiler to copy the
object as well. If this proves to be a performance bottleneck, we can do
something more clever.
Note, it's arguable that the Object struct should not allow keys to be
mutated at all. This would be tricky given the current Object interface.
Perhaps we can improve this in the future to avoid similar issues.
These changes also resolve an issue with the rewritten var set returned
by the query compiler. Previously, all seen vars were returned in the
set (as opposed to only those that had been redeclared.) When the query
compiler inverts the map before returning it could accidentally elide
vars, e.g., given {a: b, b: b} if it inverted this to {b: a} then the
output would be correct but if it inverted this to {b: b} (which it
could depending on the iteration order) the output would be incorrect.
Fixes #1125
Signed-off-by: Torin Sandall <[email protected]>
tsandall
added a commit
to tsandall/opa
that referenced
this issue
Jun 24, 2019
Mutating term values used as object keys (or set elements) is not safe because the underlying hashtables get corrupted. This a known issue that was previously fixed in the compiler (open-policy-agent#1125). The simplest solution in this case is to use the ast.Transform helper instead of ast.Walk so that a copy of object and set terms is made. Fixes open-policy-agent#1177 Signed-off-by: Torin Sandall <[email protected]>
tsandall
added a commit
that referenced
this issue
Jun 26, 2019
Mutating term values used as object keys (or set elements) is not safe because the underlying hashtables get corrupted. This a known issue that was previously fixed in the compiler (#1125). The simplest solution in this case is to use the ast.Transform helper instead of ast.Walk so that a copy of object and set terms is made. Fixes #1177 Signed-off-by: Torin Sandall <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Expected Behavior
Running this package through
opa run demo.rego,I get this:
Now, I'd expect the same output from the package
Actual Behavior
The text was updated successfully, but these errors were encountered: