lib/model: Allow updates to deleted files (fixes #3839) #3840
Conversation
|
I see this but I have to think more carefully about it than I'm capable of tonight. |
| // and not a symlink or empty space. If it's already deleted, handle | ||
| // the check inside deleteFile deleteDir only in the scenario where | ||
| // the subject we are supposed to delete actually exists. | ||
| if !fi.IsDeleted() && !osutil.IsDir(f.dir, filepath.Dir(fi.Name)) { |
There was a problem hiding this comment.
This seems dangerous, because a path that traverses Symlinks might end up in buckets. I think this allows an attacker to delete known files outside of the folder.
| // and not a symlink or empty space. If it's already deleted, handle | ||
| // the check inside deleteFile deleteDir only in the scenario where | ||
| // the subject we are supposed to delete actually exists. | ||
| if !fi.IsDeleted() && !osutil.IsDir(f.dir, filepath.Dir(fi.Name)) { |
There was a problem hiding this comment.
This change is unnecessary. (Line 515)
| // Verify that the thing we are handling lives inside a directory, | ||
| // and not a symlink or empty space. | ||
| if !osutil.IsDir(f.dir, filepath.Dir(file.Name)) { | ||
| f.newError(file.Name, "deleteDir isDir", errNotDir) |
There was a problem hiding this comment.
The delete was successful at this point, the error is pointless.
E.g. foo/bar was renamed to baz/bar and a Symlink foo -> baz was created. The Lstat check succeeds but the real file foo/bar doesn't exist anymore.
I think that the following check should be enough:
if !osutil.IsDir(f.dir, filepath.Dir(file.Name)) {
f.dbUpdates <- dbUpdateJob{file, dbUpdateDeleteDir}
return
}Lstat is not required.
| // Verify that the thing we are handling lives inside a directory, | ||
| // and not a symlink or empty space. | ||
| if !osutil.IsDir(f.dir, filepath.Dir(file.Name)) { | ||
| f.newError(file.Name, "deleteFile isDir", errNotDir) |
| @@ -1532,7 +1540,7 @@ func (f *sendReceiveFolder) dbUpdaterRoutine() { | |||
| } | |||
| lastFile = file | |||
| if err := syncFn(file); err != nil { | |||
There was a problem hiding this comment.
The above changes allow file to traverse a Symlink. You have to check with isDir, before calling syncFn. It prevents fsyncing files outside of the folder and the fsync errors disappear.
|
Instead of handling the inconsistency on the filesystem, could we fix it in the database directly? |
|
Not sure what you mean. Wasn't the point thatthe db is racey compared with the fs? |
|
Maybe I'm misunderstanding. But isn't the problem that we have:
and we get deletes for them:
so we remove both. We then get an update:
but we can't resurrect it because the dir is gone and fails the parent check? Similar thing when dir is ignored and dir/file unignored. My alternative idea for handling this is in |
|
I don't think thats the problem as we'd get the dir entry too. The issue as far as I understand it is: A and B has dir/file |
st-review
added
the
frozen-due-to-age
To be honest, I still think this somewhat contains holes.
This whole "resurrecting dir" stuff is now impossible.