Repository navigation
[16.0][FIX] fs_storage: check rooted_dir paths by whole components, without os.getcwd() - #688
gustavovalverde wants to merge 2 commits into
Conversation
Before this fix, a FileNotFoundError would ocassionally arise when 'os.getcwd()' is called due to the rooted_dir _join method to check whether the sub path is part of the parent path. This is probably caused by the number and/or combination of workers and threads and the transactions to the FS storage, having race issues. After this commit, the sub path is checked if it is contained inside the parent path virtually via posixpath and pathlib modules avoiding 'os.getcwd()' calls and running into this possible race condition.
With the root "subdirectory/here", the path "../here-backup" was accepted: it normalizes to "subdirectory/here-backup", which starts with the root as a string. A path is now accepted only when it equals the root or starts with the root followed by the separator. A root such as "/" or "C:\" already ends with the separator and keeps its sub paths. Paths are normalized as strings. On a filesystem whose separator is "/", a path must stay inside the root under both POSIX and Windows rules, since fsspec turns a backslash into a slash in local paths on Windows. A filesystem whose separator is "\" uses the Windows rules, and any other separator raises ValueError. A list or a dict of paths, which raised TypeError, is returned as joined, since DirFileSystem checks each of its paths through this method. Assisted-by: Claude Opus 5.5 Assisted-by: Claude Haiku 5.5
|
All commit authors signed the Contributor License Agreement. |
|
Is-it possible to work without IA. I'm tired to review IA.... |
I could, but it also helps that I can make contributions that have been thoroughly tested across different deployments, which would take me several hours (maybe a whole working day), instead of 30-45 minutes with the help of AI. I can make the PR's description shorter if that helps for this and further contributions though (a PR template could also help with that) |
| # A backslash is part of a name on most filesystems, but fsspec | ||
| # turns it into a slash in local paths on Windows: the path must | ||
| # stay inside the root under both rules. |
There was a problem hiding this comment.
The problem with IA is useless and obscure comment. By debugging the code I understand how it works but do you? This code deserve better explanation...
There was a problem hiding this comment.
And please don't ask to your IA. You must be able to explain how it works.
|
@gustavovalverde I proposed some changes in gustavovalverde#1 to make the code easier to understand and maintain. In my opinion, the approach made by the IA using both the POSIX and NT normalizers to normalize POSIX paths is a workaround for a limitation of the POSIX normalizer, which doesn't properly handle paths containing I've replaced this workaround with a dedicated method to handle this special case. I think this makes the code easier to read and understand. |
Summary
RootedDirFileSystem._joincompared paths as text, so it accepted a sibling of its root, such as../here-backupfor the rootsubdirectory/here. It also calledos.getcwd()throughmake_path_posix, which fails once the working directory is removed (#513). Paths are now normalized as strings and compared by whole components:Finishes #522 by @hnavarro-kernet, keeping their commit, as lmignon proposed. Fixes #513.
Review points:
/, and the input may be a list or a dict: the rules followself.sep, and an unknown separator raisesValueError. A/filesystem must also pass the Windows rules, because fsspec turns\into/in local paths on Windows. Lists and dicts are checked item by item.normpathon remote paths:make_path_posix, the source ofos.getcwd(), is no longer called._join: taken, withoutPurePath, which changes no POSIX result and adds a trailing\to UNC roots.Evidence
The new assertions on 16.0 and with this PR, both on fsspec 2026.9.0.
FileNotFoundErrormeans the path got past the check and reached the disk.All 14 fs_storage tests pass.
Merge Danger
Door: two-way. Nothing is stored or migrated.
Blast Radius: narrow.
Two kinds of paths that used to pass are now refused on every host: paths that climb with backslashes, and any sub path of a root that normalizes to
., such as/on S3, which fsspec reduces to an empty root. 17.0, 18.0, 19.0, and the 20.0 migration #670 have the same check and need forward ports.Assisted-by: Claude Opus 5.5
Assisted-by: Claude Sonnet 5.5
Assisted-by: Claude Haiku 5.5