Skip to content

Commit

Permalink
eval: prevent infinite loops and cyclical evaluation
Browse files Browse the repository at this point in the history
Avoids crashes that can happen when the stack size is exhausted
due to infinite recursion when evaluating variables.

Detect when the evaluation machinery runs into an evaluation cycle
and break the cycle by returning an empty string.
  • Loading branch information
davvid committed Feb 6, 2024
1 parent 3623201 commit 1b200ea
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 0 deletions.
5 changes: 5 additions & 0 deletions doc/src/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
variables. The first entry found is used when expanding variable expressions.
([#23](https://github.com/davvid/garden/pull/23))

- Evaluation cycles (i.e. circular variable dependencies) are now prevented when
evaluating garden variables. The evaluation engine will now return empty strings
when a variable with a cyclical expression is evaluated.
([#24](https://github.com/davvid/garden/pull/24))


## v1.2.1

Expand Down
10 changes: 10 additions & 0 deletions src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,10 @@ pub(crate) fn tree_variable(
if let Some(var_value) = var.get_value() {
return var_value.to_string();
}
if var.is_evaluating() {
return String::new();
}
var.set_evaluating(true);
let expr = var.get_expr();
let result = tree_value(
app_context,
Expand All @@ -928,6 +932,7 @@ pub(crate) fn tree_variable(
tree_name,
garden_name,
);
var.set_evaluating(false);
var.set_value(result.to_string());

result
Expand All @@ -942,8 +947,13 @@ pub(crate) fn variable(
if let Some(var_value) = var.get_value() {
return var_value.to_string();
}
if var.is_evaluating() {
return String::new();
}
var.set_evaluating(true);
let expr = var.get_expr();
let result = value(app_context, config, expr);
var.set_evaluating(false);
var.set_value(result.to_string());

result
Expand Down
13 changes: 13 additions & 0 deletions src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub type ConfigId = NodeId;
pub struct Variable {
expr: String,
value: RefCell<Option<String>>,
evaluating: RefCell<bool>,
}

impl_display_brief!(Variable);
Expand All @@ -54,6 +55,7 @@ impl Variable {
Variable {
expr,
value: RefCell::new(value),
evaluating: RefCell::new(false),
}
}

Expand All @@ -62,6 +64,17 @@ impl Variable {
self.expr.is_empty()
}

/// Is this variable currently being evaluated?
/// This is a guard variable to avoid infinite loops when evaluating.
pub fn is_evaluating(&self) -> bool {
*self.evaluating.borrow()
}

/// Set the evaluation state.
pub fn set_evaluating(&self, value: bool) {
*self.evaluating.borrow_mut() = value;
}

/// Return the raw expression for this variable.
pub fn get_expr(&self) -> &String {
&self.expr
Expand Down
14 changes: 14 additions & 0 deletions tests/data/circular.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
grafts:
graft: grafts/graft-circular.yaml

trees:
root-tree:
path: ${GARDEN_CONFIG_DIR}
variables:
root-variable: root-tree-${circular-variable}

# This file contains circular dependencies in variables.
variables:
circular-variable: ${graft::circular-variable}
root-variable: root-${circular-variable}
7 changes: 7 additions & 0 deletions tests/data/grafts/graft-circular.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
variables:
circular-variable: graft-${root-variable}

trees:
current:
path: ${GARDEN_CONFIG_DIR}
51 changes: 51 additions & 0 deletions tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,57 @@ fn eval_graft_variable_at_tree_scope() {
assert_eq!(output, "prebuilt graft value");
}

/// Test evaluating a graft variable that triggers an infinite loop via a circular dependency.
#[test]
fn eval_graft_variables_with_circular_dependencies() {
// The graft's circular-variable expression is "graft-${root-variable}".
// The root's root-variable expression is "root-${circular-variable}".
// The root's circular-variable expression is "${graft::circular-variable}".
// The short-circuit logic in the eval module returns an empty string
// so we end up with "graft-root-".
let output = garden_capture(&[
"--config",
"tests/data/circular.yaml",
"eval",
"${circular-variable}",
"graft::current",
]);
assert_eq!(output, "graft-root-");
// Introduce a cycle at the root tree scope.
let output = garden_capture(&[
"--config",
"tests/data/circular.yaml",
"eval",
"${root-variable}",
"root-tree",
]);
assert_eq!(output, "root-tree-graft-");
// Evaluate a grafted variable from the root tree's scope.
let output = garden_capture(&[
"--config",
"tests/data/circular.yaml",
"eval",
"${graft::circular-variable}",
"root-tree",
]);
assert_eq!(output, "graft-root-tree-");
// Evaluate variables at root scope without a tree scope.
let output = garden_capture(&[
"--config",
"tests/data/circular.yaml",
"eval",
"${graft::circular-variable}",
]);
assert_eq!(output, "graft-root-");
let output = garden_capture(&[
"--config",
"tests/data/circular.yaml",
"eval",
"${root-variable}",
]);
assert_eq!(output, "root-graft-");
}

/// `garden grow` creates symlinks
#[test]
#[named]
Expand Down

0 comments on commit 1b200ea

Please sign in to comment.