Access denied during Rescan triggers Delete on the remote side (Bug?)

Hi,

not sure if this is a bug or intended behaviour.

Steps to reproduce:

  • Set up Syncthing 0.13.2 on two Windows (NTFS) machines.

  • Fill a folder with some files and wait until the devices are in sync.

  • On one side, right-click on the folder, select Security and REMOVE Syncthing’s access to the folder.

-> Expected outcome: Syncthing may complain about access denied errors or something (?)

-> Actual outcome: The remote device starts deleting files. It deletes all files which the local side can no longer see due to insufficient permissions.

It seems that if Syncthing can not scan a file oder folder due to insufficient permissions, it treats it as deleted. Which is critical because the device hasn’t actually seen the delete happen. I believe that the error messages from the file system (Access denied/file not found) should be enough to distinguish between the two situations…or not?

Best Regards,

Florian Utzt

This is tricky. We may get all kinds of weird errors that mean that a file has been removed, these differ per OS, and distinguishing them with any certainty is almost impossible. So what we do is verify that the folder as a whole is still accessible (by checking the folder marker) and then considering files that can no longer be seen as deleted.

Note that this only applies when you’ve set permissions on a directory so syncthing can’t look into it to verify the existence of the files. If just a file is unreadable it will not be considered deleted.

Thanks Jakob for the insight. Now I know what happened - In my test I removed the permissions from a subdirectory, which does not contain the folder marker. Folder markers seem to be placed only on the top level of a syncthing folder, right?

Maybe a future version of Syncthing could extend this integrity check by placing the .stfolder file in every subdir? This somewhat pollutes the directory, but in comparison to a delete frenzy, it is a small price to pay…

I can guarantee that we’ll get a huge outcry from a very large number of users if we do that… Copying a folder out if a share suddenly becomes an exercise in remembering to delete the hidden marker files.

Yeah, you’re probably right. I remember that exercise from the early SVN working copies.

I’m leaning towards it being correct that we consider invisible and inaccessible files to be deleted.

Given that we presumably have some sort of “can’t access; don’t know what to do; won’t touch anything, won’t announce any changes; will warn the user” state for files we can’t access, why can’t we do the same for files inside directories we can’t access?

We could, potentially, it’s just more complex. We used to look specifically “does not exist” errors to trigger deletes, but if you for example replace the directory foo with a file foo we won’t get “does not exist” when looking for foo/bar, we’ll get a “not a directory” on Unixes and I don’t know what on Windows. I’m sure there are other corner cases.

I’ll agree though that from a safety perspective it would make more sense to do nothing than to assume deletion when a file is suddenly inaccessible.

I can’t talk for Go, but the filesystem APIs I’ve used have separate methods to check whether a file (as opposed to a directory) exists, so this doesn’t sound too bad?

Check whether file exists. If it doesn’t, it’s deleted. If it does, try and read it. If the read fails, there’s a problem. (If it’s turned into a directory then it fails the “file exists” check). If there’s a race between checking whether it was deleted and trying to read it, it’ll be marked invalid for a time, then picked up next time around. Then do the same for directories.

Or am I missing something here?

In my example above, “not a directory” is an error that gives us err.IsNotExist() => false, despite the file in question certainly not existing. I’m not sure that’s the only case with that result. Hence we adopted the policy that a stat() of a file that returns an error means the file does not exist. (An unreadable file by itself does not cause stat() to fail - it’s still clear the file exists.)

We could do the check in the other direction though; if it’s provably a “permission denied” we can be agnostic about whether it exists or not and log the error.

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.