Skip to content

Commit

Permalink
fix panic when evaluating cyclic environment references (#466)
Browse files Browse the repository at this point in the history
We track the lifetime of each environments import evaluations to detect cyclic imports. Right now this is tracked while we're evaluating the imports decl.

However, #443 introduced a way to import environment values inline. Once we've started evaluating the environment values root, we currently stop tracking whether the environment is evaluating because we're done with all the imports.

The result of this is that a cyclic environment reference will look up the environment name in the imports map and won't detect that it is still evaluating. Since the cached value in the imports map hasn't been set yet (since the environment hasn't finished evaluating) we return nil as the value. This results in a panic when we try to actually access the value.

To fix this, I'm just hoisting the evaluation lifetime tracking to the evaluation root, rather than just the imports node, so it'll correctly detect cycles for inline references.
  • Loading branch information
nyobe authored Feb 27, 2025
1 parent 7b01880 commit 36b8da2
Show file tree
Hide file tree
Showing 4 changed files with 507 additions and 4 deletions.
8 changes: 4 additions & 4 deletions eval/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,10 @@ func (e *evalContext) isReserveTopLevelKey(k string) bool {

// evaluate drives the evaluation of the evalContext's environment.
func (e *evalContext) evaluate() (*value, syntax.Diagnostics) {
mine := &imported{evaluating: true}
defer func() { mine.evaluating = false }()
e.imports[e.name] = mine

// Evaluate context. We prepare the context values to later evaluate interpolations.
e.evaluateContext()
// Evaluate imports. We do this prior to declaration so that we can plumb base values as part of declaration.
Expand Down Expand Up @@ -444,10 +448,6 @@ func (e *evalContext) evaluateContext() {

// evaluateImports evaluates an environment's imports.
func (e *evalContext) evaluateImports() {
mine := &imported{evaluating: true}
defer func() { mine.evaluating = false }()
e.imports[e.name] = mine

myImports := map[string]*value{}
for _, entry := range e.env.Imports.GetElements() {
// If the import does not have a name, there's nothing we can do. This can happen for environments
Expand Down
2 changes: 2 additions & 0 deletions eval/testdata/eval/inline-reference-cycle/bad/bad.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
values:
cycle: ${environments.bad.bad.cycle}
2 changes: 2 additions & 0 deletions eval/testdata/eval/inline-reference-cycle/env.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
imports:
- bad/bad
Loading

0 comments on commit 36b8da2

Please sign in to comment.