-
Notifications
You must be signed in to change notification settings - Fork 717
feat: allow System.FilePath.walkDir to not follow symlinks
#10973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This PR introduces a new argument to `System.FilePath.walkDir` - `followLinks` - which allows
the callee to not follow symbolic links and is set to `false` as default.
Rationale:
`System.FilePath.metadata` action follows symlinks, therefore
the branch `| ok { type := .symlink, .. }` was in fact unreachable. We
can leverage the `symlinkMetadata` action introduced in add3e1a
which uses the `lstat(2)` system call and does not follow symlinks.
This gives us the option to not follow symlinks and expose ourselves to
an infinite recursion when traversing a directory structure containing a
cycle - a situation that is currently unavoidable.
---
Rationale for setting the `followLinks := false` default:
I realize that setting the default as `false` is a breaking change, and
I don't know the internal usage of `walkDir` well enough to see if it
breaks anything - from grepping and glancing at the code, not following
symlinks seems OK.
I consulted implementations of `walkDir` in other languages and
* Python stdlib `os.walk` - https://docs.python.org/3/library/os.html#os.walk
does not follow links by default
* Rust crate `walkdir` - https://docs.rs/walkdir/latest/walkdir/struct.WalkDir.html#method.follow_links
does not follow links by default
* Go stdlib `filepath/WalkDir` - https://pkg.go.dev/path/filepath#WalkDir
does not follow links at all.
| for d in (← p.readDir) do | ||
| modify (·.push d.path) | ||
| match (← d.path.metadata.toBaseIO) with | ||
| match (← d.path.symlinkMetadata.toBaseIO) with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced in add3e1a - as per the docstring of IO.FS.FileType :
/--
Symbolic links that are pointers to other named files. Note that `System.FilePath.metadata` never
indicates this type as it follows symlinks; use `System.FilePath.symlinkMetadata` instead.
-/
it means the symlink branch could never be hit!
| let p' ← FS.realPath d.path | ||
| if (← p'.isDir) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know if it's a real issue, but these lines suffer from TOC/TOU, the first one is:
realPathaftersymlinkMetadamay fail if the symlink vanishedisDirafterrealPathmay fail if the resolved file vanished
to handle them both, it means we need to cast it to toBaseIO and catch the exceptions individually.
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
This PR introduces a new argument to
System.FilePath.walkDir-followLinks- which allows the callers to not follow symbolic links and is set tofalseas default.Closes #10972
Rationale:
System.FilePath.metadataaction follows symlinks, therefore the branch| ok { type := .symlink, .. }was in fact unreachable. We can leverage thesymlinkMetadataaction introduced in add3e1a which uses thelstat(2)system call and does not follow symlinks.This gives us the option to not follow symlinks and expose ourselves to an infinite recursion when traversing a directory structure containing a cycle - a situation that is currently unavoidable.
Rationale for setting the
followLinks := falsedefault:I realize that setting the default as
falseis a breaking change, and I don't know the internal usage ofwalkDirwell enough to see if it breaks anything - from grepping and glancing at the code, not following symlinks seems OK.I consulted implementations of
walkDirin other languages andos.walk- https://docs.python.org/3/library/os.html#os.walk does not follow links by defaultwalkdir- https://docs.rs/walkdir/latest/walkdir/struct.WalkDir.html#method.follow_links does not follow links by defaultfilepath/WalkDir- https://pkg.go.dev/path/filepath#WalkDir does not follow links at all.