[5.6] FileLock encounters a runtime error when the path of the locked file is long (#277) #278
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…is long FileLock constructs the name of the lock file based on the path of the file being locked. When this path is very long, the name of the constructed lock file exceeds `NAME_MAX` (or more specifically the directory entry name length limit of the file system on which the directory that contains it resides, but `NAME_MAX` is a decent approximation on modern file systems commonly in use). This change truncates it to `NAME_MAX` UTF-8 bytes (starting from the end), rounding down to avoid splitting Unicode scalars (but making no effort to avoid splitting clusters, since they don't affect the validity of the filename). Another possible refinement would be to also include a hash of the elided part of the filename — in the interest of avoiding complexity this implementation doesn't do that, and it's not clear that it's needed given that 255 (the common value of `NAME_MAX`) usually results in a unique enough name in any practical circumstances. rdar://87471360 (cherry picked from commit 2a2ba2a)
abertelrud
requested review from
aciidgh,
friedbunny,
neonichu and
tomerd
as code owners
|
@swift-ci please test |
tomerd
approved these changes
Jan 13, 2022
|
Linux: |
|
@swift-ci please test linux |
|
Will need to resolve the Windows issue identified in #280 before merging. I really wish we had some kind of Windows PR testing to flag these issues. |
This fixes multiple issues with the file locking implementation on Windows. `AbsolutePath.root` is meaningless on Windows - there is no concept of a singular root on Windows, instead you have have 26 individual roots - A-Z. Stripping the count of characters for the root path leaves us with an invalid path string as the root drives have been stripped. The next character is an invalid character on most file systems, and so we must replace `:`. Instead of using a `MAX_NAME` to compute a length of the ARC, enforce a limit for the complete path (though an ARC would be limited to 255 codepoints on NTFS). This incidentally also corrects the emitted path, previous path computation left us with the file in PWD.
|
@swift-ci please test |
abertelrud
added
swift 5.6
tomerd
approved these changes
Jan 14, 2022
airspeedswift
approved these changes
Jan 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is cherry-picked from #277.
Explanation: FileLock constructs the name of the lock file based on the path of the file being locked. When this path is very long, the name of the constructed lock file exceeds
NAME_MAX(or more specifically the directory entry name length limit of the file system on which the directory that contains it resides, butNAME_MAXis a decent approximation on modern file systems commonly in use).Scope of Issue: This can affect any use of the file locking where the locked file is at a long path (i.e. deeply nested and/or with long path components). This issue was seen in SwiftPM.
Reason for Nominating to 5.6: This causes unit test failures in SwiftPM and can cause runtime failures in actual use (typically not crashes but errors such as build errors and other kinds of failures). Although this can be worked around by moving the packages to a shallower path, it's not at all obvious that this is the problem nor is it always possibly to affect the paths of for example CI systems.
Risk: Low — the problem is understood and the fix is straightforward. The main issue would be if there is something special about Unicode content of paths that hasn't been accounted for. Any failure to fix the problem should be evident from continued failure of unit tests such as in SwiftPM and its client IDEs.
Reviewed By: @tomerd, @neonichu
Automated Testing: An updated unit test checks for this condition.
Dependencies: None
Impact on CI: None
How to Verify: Create a package that uses this API and try to lock a path whose total length in UTF-8 bytes is greater than the value of
NAME_MAX(this is also what the unit test does).rdar://87471360