Use SHCreateDirectoryExW to create a directory with intermediate directories on Windows

[ahoppen]

Instead of recursively creating all the parent directories, which is racy if the directory is created between the check if the parent directory exists and the actual directory creation, use SHCreateDirectoryExW, which also creates intermediate directories.

Alternative to #1021

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[ahoppen]

Ah, looks like SHCreateDirectoryExW only works on fully qualified paths and not relative paths. I guess we could form a fully-qualified path for SHCreateDirectoryExW but I’m a little worried because now the implementation for withIntermediateDirectories: true starts to deviate from the one with withIntermediateDirectories: false quite significantly…

[ahoppen]

OK, after some more thinking, maybe using SHCreateDirectoryExW is better than recursively walking up the path and creating all intermediate directories manually, if only because there are fewer atomic operations that can race with other file system operations.

The Gist for creating a directory with intermediate directories is now:

• If the path is not absolute, make it absolute using URL(filePath: path, relativeTo: self.currentDirectoryPath) because SHCreateDirectoryExW requires an absolute path
• Create the directory
• If creating the directory failed but we have a directory at that path already, return success
  • Interestingly, I noticed that we don’t apply attributes to the directory in this case. Not sure if that’s intended behavior.
• Otherwise return failure

We have two different states that we store in this implementation:

• The current directory. But this shouldn’t cause any race conditions because if the current directory changes after we retrieve it but before we create the directory, the behavior is the same as if the current directory was changed after directory creation finished
• The error code from directory creation and the fileExists check. But this should be fine because the only situation in which we modify behavior based on fileExists if it returns true and has isDirectory == true. At this point it doesn’t really matter what directory creation did. We have a directory and that’s the intended outcome, so we can return success without any issues.

One thing that I’m not entire sure about is whether SHCreateDirectoryExW might have subtly different behavior to CreateDirectoryW (which we use if we don’t want to create intermediate directories), which could cause issues in the future. @compnerd Do you think this is a concern? You have more experience with the Windows API. Fo example, one thing that I noticed is that the documentation of SHCreateDirectoryExW says:

SHCreateDirectoryEx also verifies that the files are visible. If they are not visible, expect one of the following:

And I’m not exactly sure what that means. I was able to create directories inside hidden folders alright, so that doesn’t seem to be it. But really I’m trying to figure out if there will be a bunch of these subtle differences or whether SHCreateDirectoryExW and CreateDirectoryW should behave mostly the same.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[jmschonfeld]

Thanks so much for iterating on this - I personally like this approach and am in favor of it over the added fileExists check to the existing implementation assuming @compnerd doesn't see any issues with using SHCreateDirectoryExW. I agree with your assessment on the possible asynchronous state changing that I think based on the nature of it here, it doesn't matter and wouldn't result in a true TOCTOU issue. Thanks!!

[compnerd]

When exactly can this happen? Unlike Unix, the current path cannot be removed from under you as you have an open handle. The path cannot have been constructed after you entered as it would not exist. This feels like we could precondition it.

[ahoppen]

I’m not sure when there would be no working directory on Windows. GetCurrentDirectory says

If the function fails, the return value is zero. To get extended error information, call GetLastError.

Which leads me to believe that there are ways for it to fail.

Happy to change this to a precondition though. Crashing is probably preferable to doing unknown file system operations.

While looking into this, I also noticed that currentDirectoryPath can be nil due to a race condition, but I don’t think we need to cover that here: #1034

[compnerd]

Is the else necessary? Can we not just drop the else and undent the body of the clause?

[ahoppen]

Yeah, can be flattened

[ahoppen]

@swift-ci Please test