fix(forge): fix stack overflow when the lib path is absolute.

[l1nxy]

Motivation

Close #9127
The crash is caused by the recursive call of get_mapping in the config crate:

foundry/crates/config/src/providers/remappings.rs

Lines 101 to 161 in c6d59b3

	fn get_remappings(&self, remappings: Vec<Remapping>) -> Result<Vec<Remapping>, Error> {
	trace!("get all remappings from {:?}", self.root);
	/// prioritizes remappings that are closer: shorter `path`
	/// - ("a", "1/2") over ("a", "1/2/3")
	///
	/// grouped by remapping context
	fn insert_closest(
	mappings: &mut BTreeMap<Option<String>, BTreeMap<String, PathBuf>>,
	context: Option<String>,
	key: String,
	path: PathBuf,
	) {
	let context_mappings = mappings.entry(context).or_default();
	match context_mappings.entry(key) {
	Entry::Occupied(mut e) => {
	if e.get().components().count() > path.components().count() {
	e.insert(path);
	}
	}
	Entry::Vacant(e) => {
	e.insert(path);
	}
	}
	}
	// Let's first just extend the remappings with the ones that were passed in,
	// without any filtering.
	let mut user_remappings = Vec::new();
	// check env vars
	if let Some(env_remappings) = remappings_from_env_var("DAPP_REMAPPINGS")
	.or_else(|| remappings_from_env_var("FOUNDRY_REMAPPINGS"))
	{
	user_remappings
	.extend(env_remappings.map_err::<Error, _>(|err| err.to_string().into())?);
	}
	// check remappings.txt file
	let remappings_file = self.root.join("remappings.txt");
	if remappings_file.is_file() {
	let content = fs::read_to_string(remappings_file).map_err(|err| err.to_string())?;
	let remappings_from_file: Result<Vec<_>, _> =
	remappings_from_newline(&content).collect();
	user_remappings
	.extend(remappings_from_file.map_err::<Error, _>(|err| err.to_string().into())?);
	}
	user_remappings.extend(remappings);
	// Let's now use the wrapper to conditionally extend the remappings with the autodetected
	// ones. We want to avoid duplicates, and the wrapper will handle this for us.
	let mut all_remappings = Remappings::new_with_remappings(user_remappings);
	// scan all library dirs and autodetect remappings
	// TODO: if a lib specifies contexts for remappings manually, we need to figure out how to
	// resolve that
	if self.auto_detect_remappings {
	let mut lib_remappings = BTreeMap::new();
	// find all remappings of from libs that use a foundry.toml
	for r in self.lib_foundry_toml_remappings() {
	insert_closest(&mut lib_remappings, r.context, r.name, r.path.into());
	}

The get_mapping function calls the lib_foundry_toml_remappings function:

foundry/crates/config/src/providers/remappings.rs

Lines 196 to 211 in c6d59b3

	fn lib_foundry_toml_remappings(&self) -> impl Iterator<Item = Remapping> + '_ {
	self.lib_paths
	.iter()
	.map(|p| self.root.join(p))
	.flat_map(foundry_toml_dirs)
	.inspect(|lib| {
	trace!("find all remappings of nested foundry.toml lib: {:?}", lib);
	})
	.flat_map(|lib: PathBuf| {
	// load config, of the nested lib if it exists
	let config = Config::load_with_root(&lib).sanitized();
	// if the configured _src_ directory is set to something that
	// [Remapping::find_many()] doesn't classify as a src directory (src, contracts,
	// lib), then we need to manually add a remapping here
	let mut src_remapping = None;

In the lib_foundry_toml_remappings, it reads config from a nested lib folder. However, when the lib path in self.lib_paths is absolute, it uses self.root.join(lib_path), it will produce wrong path if lib_path is absolute.
The join function is described as follows:

pub fn join<P: AsRef<Path>>(&self, path: P) -> PathBuf
Creates an owned PathBuf with path adjoined to self.
If the path is absolute, it replaces the current path.

Therefore, if the lib paths are absolute, they will only return the setting path and recursively search for nested libs.

Solution

I propose adding a check to determine whether the lib path is absolute or not. If it is absolute, we should return the root folder joined by lib. Otherwise, we should return just the new path based on lib's relative path.
By the way, I am not sure if setting an absolute path for the lib option in config is allowed. If we need to disable it, we can do so and print a warning message to inform users.

[zerosnacks]

Hi @l1nxy thanks for your PR!

Would you mind adding a test case for this?

We have some tests for remappings here as a reference:

foundry/crates/forge/tests/cli/config.rs

Lines 494 to 594 in ab8ebf6

	// test that we can detect remappings from foundry.toml
	forgetest_init!(can_detect_lib_foundry_toml, |prj, cmd| {
	let config = cmd.config();
	let remappings = config.remappings.iter().cloned().map(Remapping::from).collect::<Vec<_>>();
	similar_asserts::assert_eq!(
	remappings,
	vec![
	// global
	"forge-std/=lib/forge-std/src/".parse().unwrap(),
	]
	);
	// create a new lib directly in the `lib` folder with a remapping
	let mut config = config;
	config.remappings = vec![Remapping::from_str("nested/=lib/nested").unwrap().into()];
	let nested = prj.paths().libraries[0].join("nested-lib");
	pretty_err(&nested, fs::create_dir_all(&nested));
	let toml_file = nested.join("foundry.toml");
	pretty_err(&toml_file, fs::write(&toml_file, config.to_string_pretty().unwrap()));
	let config = cmd.config();
	let remappings = config.remappings.iter().cloned().map(Remapping::from).collect::<Vec<_>>();
	similar_asserts::assert_eq!(
	remappings,
	vec![
	// default
	"forge-std/=lib/forge-std/src/".parse().unwrap(),
	// remapping is local to the lib
	"nested-lib/=lib/nested-lib/src/".parse().unwrap(),
	// global
	"nested/=lib/nested-lib/lib/nested/".parse().unwrap(),
	]
	);
	// nest another lib under the already nested lib
	let mut config = config;
	config.remappings = vec![Remapping::from_str("nested-twice/=lib/nested-twice").unwrap().into()];
	let nested = nested.join("lib/another-lib");
	pretty_err(&nested, fs::create_dir_all(&nested));
	let toml_file = nested.join("foundry.toml");
	pretty_err(&toml_file, fs::write(&toml_file, config.to_string_pretty().unwrap()));
	let another_config = cmd.config();
	let remappings =
	another_config.remappings.iter().cloned().map(Remapping::from).collect::<Vec<_>>();
	similar_asserts::assert_eq!(
	remappings,
	vec![
	// local to the lib
	"another-lib/=lib/nested-lib/lib/another-lib/src/".parse().unwrap(),
	// global
	"forge-std/=lib/forge-std/src/".parse().unwrap(),
	"nested-lib/=lib/nested-lib/src/".parse().unwrap(),
	// remappings local to the lib
	"nested-twice/=lib/nested-lib/lib/another-lib/lib/nested-twice/".parse().unwrap(),
	"nested/=lib/nested-lib/lib/nested/".parse().unwrap(),
	]
	);
	config.src = "custom-source-dir".into();
	pretty_err(&toml_file, fs::write(&toml_file, config.to_string_pretty().unwrap()));
	let config = cmd.config();
	let remappings = config.remappings.iter().cloned().map(Remapping::from).collect::<Vec<_>>();
	similar_asserts::assert_eq!(
	remappings,
	vec![
	// local to the lib
	"another-lib/=lib/nested-lib/lib/another-lib/custom-source-dir/".parse().unwrap(),
	// global
	"forge-std/=lib/forge-std/src/".parse().unwrap(),
	"nested-lib/=lib/nested-lib/src/".parse().unwrap(),
	// remappings local to the lib
	"nested-twice/=lib/nested-lib/lib/another-lib/lib/nested-twice/".parse().unwrap(),
	"nested/=lib/nested-lib/lib/nested/".parse().unwrap(),
	]
	);
	});
	// test remappings with closer paths are prioritised
	// so that `dep/=lib/a/src` will take precedent over `dep/=lib/a/lib/b/src`
	forgetest_init!(can_prioritise_closer_lib_remappings, |prj, cmd| {
	let config = cmd.config();
	// create a new lib directly in the `lib` folder with conflicting remapping `forge-std/`
	let mut config = config;
	config.remappings = vec![Remapping::from_str("forge-std/=lib/forge-std/src/").unwrap().into()];
	let nested = prj.paths().libraries[0].join("dep1");
	pretty_err(&nested, fs::create_dir_all(&nested));
	let toml_file = nested.join("foundry.toml");
	pretty_err(&toml_file, fs::write(&toml_file, config.to_string_pretty().unwrap()));
	let config = cmd.config();
	let remappings = config.get_all_remappings().collect::<Vec<_>>();
	similar_asserts::assert_eq!(
	remappings,
	vec![
	"dep1/=lib/dep1/src/".parse().unwrap(),
	"forge-std/=lib/forge-std/src/".parse().unwrap()
	]
	);
	});

[l1nxy]

@mattsse ping

[mattsse]

makes sense, imo absolute paths should be discouraged