Fix a corner case where createDirectory would fail on windows #1479
Conversation
daveinglis
force-pushed
the
fix_long_window_paths
branch
from
f0af396 to
d5ffb8a
Compare
|
@swift-ci please test |
daveinglis
force-pushed
the
fix_long_window_paths
branch
from
d5ffb8a to
1bf3406
Compare
|
@swift-ci please test |
daveinglis
force-pushed
the
fix_long_window_paths
branch
from
1bf3406 to
9192a96
Compare
|
@swift-ci please test |
| func createDirectoryRecursively(at directoryPath: String) throws { | ||
| // Check if directory already exists | ||
| var isDirectory: Bool = false | ||
| if fileExists(atPath: directoryPath, isDirectory: &isDirectory) { |
There was a problem hiding this comment.
I think this could open us up to TOC-TOU issues. Since we're already handling ERROR_ALREADY_EXISTS below when we try to create the directory, can we just elide this check and let the error handling of CreateDirectoryW take care of the rest? (I know the error handling below also feels TOC-TOU-like, but given we can't know if the pre-existing result was a directory in the same call, it seems better to have a TOC-TOU issue when deciding whether to throw the error than deciding whether to create the directory to begin with)
There was a problem hiding this comment.
yep, that makes sense, will update.
There was a problem hiding this comment.
so Im rethinking this.... This first check is recursively walking the parent looking for where to start creating the folders, without it, it would always walk to the root, creating the folders from there, this seems it could be quite the performance hit if this first check is removed, would it not?
There was a problem hiding this comment.
The previous implementation didn't have this check, right (just confirming it's not a performance regression from a previous release to remove it)? I agree it would be more work, but it seems like that's probably correct work to do since the system doesn't have an API that does this in one call atomically, right? As unfortunate as performing a bit more work is, I think minimizing the potential security issues that typically come with TOC-TOU discrepancies is important here even if it means we need to do more work because we can't check for the optimal case first.
There was a problem hiding this comment.
@jmschonfeld Ok, I have updated this to just use the CreateDirectoryW result to determine if it already exists walking done the tree, then back up once it exists or is created.
Tests/FoundationEssentialsTests/FileManager/FileManagerTests.swift
Outdated
Show resolved
Hide resolved
Sources/FoundationEssentials/FileManager/FileManager+Directories.swift
Outdated
Show resolved
Hide resolved
daveinglis
force-pushed
the
fix_long_window_paths
branch
from
9192a96 to
e8e3ead
Compare
|
@swift-ci please test |
daveinglis
force-pushed
the
fix_long_window_paths
branch
from
e8e3ead to
d9a17bb
Compare
|
@swift-ci please test |
| @@ -66,7 +66,8 @@ extension String { | |||
| // 2. Canonicalize the path. | |||
| // This will add the \\?\ prefix if needed based on the path's length. | |||
| var pwszCanonicalPath: LPWSTR? | |||
| let flags: ULONG = PATHCCH_ALLOW_LONG_PATHS | |||
| // Alway add the long path prefix since we don't know if this is a directory. | |||
| let flags: ULONG = PATHCCH_ENSURE_IS_EXTENDED_LENGTH_PATH | |||
There was a problem hiding this comment.
Do we not also need the PATHCCH_ALLOW_LONG_PATHS flag anymore?
@daveinglis Also, the same code is copied to:
- swift-system
- swift-build
- swift-subprocess
There are also variations in swift-tools-support-core and swift-testing.
Can you open PRs to make the change there as well?
There was a problem hiding this comment.
The PATHCCH_ALLOW_LONG_PATHS is not needed, PATHCCH_ENSURE_IS_EXTENDED_LENGTH_PATH will always prepend the //?/
There was a problem hiding this comment.
So I saw the swift-build version, but it's never used in the context of creating a directory so should work fine as is. I can look at the other packages but if the usage is never used with creating directories do we still need it changed?
There was a problem hiding this comment.
So I saw the swift-build version, but it's never used in the context of creating a directory so should work fine as is. I can look at the other packages but if the usage is never used with creating directories do we still need it changed?
You never know when it might be used in new contexts, and because of cargo-culting I think we want to ensure that the most-correct implementation is kept in sync across all places it's copied to.
Ideally we'd have one function that everything could call into, but not a great place for this sort of SPI...
There was a problem hiding this comment.
ok, I will start putting up some PR for the other version so that all match.
The //?/ prefix needs to be added to directories if the length is greater than MAX_PATH - 12, however the PATHCCH_ALLOW_LONG_PATHS to PathAllocCanonicalize would only do that if the path was greater than MAX_PATH. This change uses PATHCCH_ENSURE_IS_EXTENDED_LENGTH_PATH, which forces it on all the time. Just needed to remove the prefix in a few places as it was making it way out into tests as this is suppose to be transparent to the user. This also fixes createDirectory() "withIntermediateDirectories: true" for long paths on Windows.
daveinglis
force-pushed
the
fix_long_window_paths
branch
from
d9a17bb to
3ec5ddc
Compare
|
@swift-ci please test |
The //?/ prefix needs to be added to directories if the length is greater than MAX_PATH - 12, however the PATHCCH_ALLOW_LONG_PATHS to PathAllocCanonicalize would only do that if the path was greater than MAX_PATH. This change uses PATHCCH_ENSURE_IS_EXTENDED_LENGTH_PATH, which forces it on all the time. Just needed to remove the prefix in a few places as it was making it way out into tests as this is suppose to be transparent to the user.
This also fixes createDirectory()
withIntermediateDirectories: truefor long paths on Windows.closes #1486