Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Expand fails to work recursively with some combinations #2387

Closed
ranweiler opened this issue Sep 13, 2022 · 2 comments · Fixed by #2789
Closed

Expand fails to work recursively with some combinations #2387

ranweiler opened this issue Sep 13, 2022 · 2 comments · Fixed by #2789
Assignees
Labels
bug Something isn't working

Comments

@ranweiler
Copy link
Member

ranweiler commented Sep 13, 2022

The Expand evaluator canonicalizes Path-typed values eagerly, since #22. Canonicalization is an I/O operation, and fails if the resulting path does not exist on-disk. As a result, if a Path-valued string is expanded to a value that containers another Expand placeholder (expandable variable), the expansion will fail.

This is exhibited by an important use case that currently fails:

#[test]
fn test_setup_dir_and_target_exe() -> Result<()> {
    let expand = Expand::new()
        .setup_dir("/my/setup")
        .target_exe("{setup_dir}/my-fuzzer.exe");

    let target_exe = expand.evaluate_value("{target_exe}")?;
    assert_eq!(target_exe, "/my/setup/my-fuzzer.exe.);

    Ok(())
}

In the above, the internal expansions should be:

  1. {target_exe}
  2. {setup_dir}/my-fuzzer.exe
  3. /my/setup/my-fuzzer.exe

But, since the intermediate state of (2) is an ExpandedValue::Path and does not exist on disk due to the unexpanded path segment, the call to canonicalize() in Expand::replace_value() fails.

The fix to this must support the above test case, and maybe reasonably abandon canonicalization (or at least have a mode that permits canonicalization failures). It must also avoid expansion cycles that would lead to stack overflows or infinite loops.

AB#41312795

@Porges
Copy link
Member

Porges commented Sep 20, 2022

@ranweiler Is there any reason to canonicalize during expansion and not once at the end?

@ranweiler
Copy link
Member Author

@ranweiler Is there any reason to canonicalize during expansion and not once at the end?

I think that would work fine (though I'm not sure we really should continue to require that the final paths exist at expansion time).

The trick is just that canonicalize() is currently called for the value of placeholder variables assignments of type ExpandValue::Path. But its caller, evaluate_value(), doesn't know if the ultimately-returned String is itself actually a Path. It truly isn't, in general. It might be an env var assignment, or a UUID, &c. So we could just change the API and require that callers canonicalize as appropriate, rather than promising to canonicalize values. We could perhaps add a helper expand_path() that calls expand_value() and then canonicalizes for callers.

Porges added a commit that referenced this issue Jan 31, 2023
- Recursively expand Paths before canonicalization; fixes #2387
- Detect and error on infinitely-expanding variables (due to self-reference or mutual recursion); this fixes an existing problem with list expansion as well
- Detect and report references to unknown replacements (e.g. typoes)
- Detect and report references to _known_ replacements which are not available

---

There is a behavioural change here which is that values which were previously unknown such as `{x}` were unexpanded; from now on these will be errors. It's possible that something somewhere is relying on this.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants