Skip to content

Commit

Permalink
Implement reference loop detection
Browse files Browse the repository at this point in the history
We add logic to track any reference paths which we've already seen while
resolving references in a `Value`. Whenever we encounter a reference
path which we've already seen before, we abort reference resolution and
provide the list of paths which form a loop in the error message.

Note that the already introduced boundaries where we don't need to track
paths seen by recursive calls (across ValueList layers, between Sequence
elements and between Mapping key-value pairs) still apply.

We also leave the recursion depth limit in place for now as a safeguard
for potential bugs in the loop detection.

The existing tests are updated to reflect the changed error messages for
the test cases which contain reference loops.
  • Loading branch information
simu committed Sep 13, 2023
1 parent 6ec652f commit 0ab1953
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 8 deletions.
28 changes: 27 additions & 1 deletion src/refs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ mod parser;
use crate::types::{Mapping, Value};
use anyhow::{anyhow, Result};
use nom::error::{convert_error, VerboseError};
use std::collections::HashSet;

#[derive(Debug, PartialEq, Eq)]
/// Represents a parsed Reclass reference
Expand All @@ -18,11 +19,26 @@ pub enum Token {

#[derive(Clone, Debug, Default)]
pub struct ResolveState {
/// Reference paths which we've seen during reference resolution
seen_paths: HashSet<String>,
/// Recursion depth of the resolution (in number of calls to Token::resolve() for Token::Ref
/// objects).
depth: usize,
}

impl ResolveState {
/// Formats paths that have been seen as a comma-separated list.
fn seen_paths_list(&self) -> String {
let mut paths = self
.seen_paths
.iter()
.map(|p| format!("\"{p}\""))
.collect::<Vec<String>>();
paths.sort();
paths.join(", ")
}
}

/// Maximum allowed recursion depth for Token::resolve(). We're fairly conservative with the value,
/// since it's rather unlikely that a well-formed inventory will have any references that are
/// nested deeper than 64.
Expand Down Expand Up @@ -98,14 +114,24 @@ impl Token {
// implementation. We abort at a recursion depth of 64, since it's quite
// unlikely that there's a legitimate case where we have a recursion depth of
// 64 when resolving references for a well formed inventory.
let paths = state.seen_paths_list();
return Err(anyhow!(
"Token resolution exceeded recursion depth of {RESOLVE_MAX_DEPTH}."
"Token resolution exceeded recursion depth of {RESOLVE_MAX_DEPTH}. \
We've seen the following reference paths: [{paths}].",
));
}
// Construct flattened ref path by resolving any potential nested references in the
// Ref's Vec<Token>.
let path = interpolate_token_slice(parts, params, state)?;

if state.seen_paths.contains(&path) {
// we've already seen this reference, so we know there's a loop, and can abort
// resolution.
let paths = state.seen_paths_list();
return Err(anyhow!("Reference loop with reference paths [{paths}]."));
}
state.seen_paths.insert(path.clone());

// generate iterator containing flattened reference path segments
let mut refpath_iter = path.split(':');
// we handle the first element separately, so we can establish a local mutable
Expand Down
6 changes: 3 additions & 3 deletions src/refs/token_resolve_parse_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ fn test_resolve_mapping_embedded() {
}

#[test]
#[should_panic(expected = "Token resolution exceeded recursion depth of 64.")]
#[should_panic(expected = "Reference loop with reference paths [\"foo\"].")]
fn test_resolve_recursive_error() {
let p = r#"
foo: ${foo}
Expand All @@ -235,7 +235,7 @@ fn test_resolve_recursive_error() {
}

#[test]
#[should_panic(expected = "Token resolution exceeded recursion depth of 64.")]
#[should_panic(expected = "Reference loop with reference paths [\"bar\", \"foo\"].")]
fn test_resolve_recursive_error_2() {
let p = r#"
foo: ${bar}
Expand All @@ -249,7 +249,7 @@ fn test_resolve_recursive_error_2() {
}

#[test]
#[should_panic(expected = "Token resolution exceeded recursion depth of 64.")]
#[should_panic(expected = "Reference loop with reference paths [\"baz\", \"foo\"].")]
fn test_resolve_nested_recursive_error() {
let p = r#"
foo: ${baz}
Expand Down
9 changes: 5 additions & 4 deletions src/types/value/value_interpolate_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ fn test_interpolate_nested_mapping_no_loop() {
#[test]
#[should_panic(expected = "While resolving references in \
{\"foo\": {\"bar\": \"${bar}\"}, \"bar\": [{\"baz\": \"baz\", \"qux\": \"qux\"}, \
{\"baz\": \"${foo}\"}]}: Token resolution exceeded recursion depth of 64.")]
{\"baz\": \"${foo}\"}]}: Reference loop with reference paths [\"bar\", \"foo\"].")]
fn test_merge_interpolate_loop() {
let base = r#"
foo:
Expand Down Expand Up @@ -499,7 +499,7 @@ fn test_merge_interpolate_loop() {
#[should_panic(expected = "While resolving references in \
{\"foo\": {\"bar\": [\"${bar}\", \"${baz}\"]}, \"bar\": \"${qux}\", \
\"baz\": {\"bar\": \"${foo}\"}, \"qux\": 3.14}: \
Token resolution exceeded recursion depth of 64.")]
Reference loop with reference paths [\"baz\", \"foo\"].")]
fn test_interpolate_sequence_loop() {
let base = r#"
foo:
Expand All @@ -522,7 +522,7 @@ fn test_interpolate_sequence_loop() {
{\"foo\": {\"bar\": {\"baz\": \"${foo:baz:bar}\", \"qux\": \"${foo:qux:foo}\"}, \
\"baz\": {\"bar\": \"qux\", \"qux\": \"${foo:bar:qux}\"}, \"qux\": \
{\"foo\": \"${foo:baz:qux}\"}}}: \
Token resolution exceeded recursion depth of 64.")]
Reference loop with reference paths [\"foo:bar:qux\", \"foo:baz:qux\", \"foo:qux:foo\"].")]
fn test_interpolate_nested_mapping_loop() {
let m = r#"
foo:
Expand Down Expand Up @@ -551,7 +551,8 @@ fn test_interpolate_nested_mapping_loop() {
${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:\
${foo:${foo:${foo:${foo:${foo:${foo}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}\
}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}\": \
Token resolution exceeded recursion depth of 64."
Token resolution exceeded recursion depth of 64. \
We've seen the following reference paths: []."
)]
fn test_interpolate_depth_exceeded() {
// construct a reference string which is a nested sequence of ${foo:....${foo}} with 70 nesting
Expand Down

0 comments on commit 0ab1953

Please sign in to comment.