Fix potential crash in repository cache fallback #3221
Conversation
neonichu
requested review from
abertelrud,
friedbunny and
tomerd
as code owners
It is possible that copying from the cache fails mid-way which can lead to SwiftPM hitting a precondition in the fallback. We should clear any potential leftover data before doing the fallback. rdar://73505115
neonichu
force-pushed
the
fix-potential-crash-in-fallback
branch
from
72b6ed9 to
885218a
Compare
| @@ -384,6 +384,8 @@ public class RepositoryManager { | |||
| } catch { | |||
| // Fetch without populating the cache in the case of an error. | |||
| print("Skipping cache due to an error: \(error)") | |||
| // It is possible that we already created the directory before failing, so clear leftover data if present. | |||
| try fileSystem.removeFileTree(repositoryPath) | |||
There was a problem hiding this comment.
I'm pretty sure that this can't spuriously fail because repositoryPath is missing, but could it fail because its parent path is missing or other reason? i.e. would it be better to deal with exceptions from here or let them raise?
There was a problem hiding this comment.
testing if it exists before deleting sounds reasonable, then raising if we can't delete otherwise we will have downstream issues?
There was a problem hiding this comment.
It's probably not a problem in this particular case, but in general, checking for existence first can be prone to race conditions. So a pattern that usually works well for cases like this is to just do the file system operation but to silently ignore benign problems. In this case, an ENOENT error is fine but anything else should be reported. That might be what the underlying API already does, so my question was just to confirm that semantic. I'll check the code.
There was a problem hiding this comment.
Today it does this:
if self.exists(path, followSymlink: false) {
try FileManager.default.removeItem(atPath: path.pathString)
}
So the race potential is there.
There was a problem hiding this comment.
That's possibly a follow-on fix down in TSC, but it looks as if the code in this particular PR is correct (up in SwiftPM). I seems like correct behavior that FileSystem.removeFileTree(repositoryPath) doesn't throw an error if the file isn't already there (much like mkdir doesn't complain if the directory is there). So the code in this PR is right but TSC could use a fix, it looks like. So I have no concerns about this PR. Thanks!
There was a problem hiding this comment.
I think what would make the most sense is to leave this as-is but change removeFileTree in TSC to not throw on ENOENT as suggested, but for the 5.4 cherry-pick we inline the ENOENT check to reduce the surface area of the change.
|
@swift-ci please smoke test |
|
Starting smoke tests. This would be good to get into 5.4 is possible; it definitely seems safe enough. |
Motivation:
It is possible that copying from the cache fails mid-way which can lead to SwiftPM hitting a precondition in the fallback.
Modifications:
We should clear any potential leftover data before doing the fallback.
Result:
Hitting the precondition can be avoided.
rdar://73505115