Log infinite symlink offender #20985
Conversation
huonw
added
category:documentation
release-notes:not-required
ndellosa95
changed the title
src/rust/engine/fs/src/directory.rs
Outdated
| let destination_path = if s.target == Component::CurDir.as_os_str() { None } else { Some(path_so_far.join(s.target.clone())) }; | ||
| let destination_entry = root.entry_helper(root, destination_path.as_ref().unwrap_or(&path), link_depth + 1); |
There was a problem hiding this comment.
@thejcannon you added this code originally. Can you validate that just eliding a . self symlink (and dropping the warning) seems reasonable, rather than repeatedly traversing it?
There was a problem hiding this comment.
I think it may actually make sense to repeatedly traverse up to the link_depth but it seems like the logic for what gets traversed is inconsistent. If you look at the assertions for walk_too_many_links_rootdir for files we have (0..MAX_LINK_DEPTH).map(|n| ("self/".repeat(n.into()) + "file.txt")).collect::<Vec<_>>() but for directories it's just vec!["".to_string()]. So it seems like the files appear before the symlink and the directory appears afterwards in the traversal. But the reasoning behind this logic is unclear to me, nor is it clear to that this logic always holds (i.e. files always come first, then symlinks, then directories) or is just due to the way the tests are set up.
There was a problem hiding this comment.
A maybe more illustrative example is if you have a digest with a symlink self -> . and a subdirectory a with a file a/file.txt and a symlink a/self -> . then the contents of that subdirectory will never get traversed because it will just get hung up on the first infinite symlink.
There was a problem hiding this comment.
I can't think of reason why that wouldn't be ok. And certainly if it was something we need to support the test/comments should spell that out
There was a problem hiding this comment.
I can't think of reason why that wouldn't be ok. And certainly if it was something we need to support the test/comments should spell that out
@thejcannon Sorry do you mean just eliding the self -> . symlink is okay or the potential issue I'm calling out about directory contents potentially never getting traversed due to the self -> . symlink is not actually an issue?
Just eliding actually fails the tests because they expect a file named self/self/self.../file.txt. It also fails the entry_self_referencing_symlink_subdir test because we now have a directory named a/self/self/self... instead of just a.
There was a problem hiding this comment.
Ah right. Good thing past josh wrote tests
There was a problem hiding this comment.
Can you explain the rationale behind having the file a/self/self.../file.txt but not the directories a/self/self...?
There was a problem hiding this comment.
I think it depends on if the capture is symlink aware or not, doesn't it?
There was a problem hiding this comment.
Right now it seems like it's only oblivious at the file-level though? I don't really get it but I'll trust that the unit testing logic is correct and have reverted to the original implementation except quickly constructing the logical path for better error messaging when self -> . occurs.
Gentle status ping; @huonw is this good to land? |
Gave this a stab since I have infinite free time now,
but I have no idea how to see the warning output in tests.Figured out the (seemingly) only way to see log output in tests is by using the
env_loggercrate and running with theRUST_LOG=warnenv var set. The following command spits out the subsequent log messages in the tests:RUST_LOG=warn ./cargo test -p fs -- --nocapture