Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filesystem watching failed when parent folder is not listable #5609

Closed
gdlmx opened this issue Mar 21, 2019 · 15 comments

Comments

Projects
None yet
4 participants
@gdlmx
Copy link
Contributor

commented Mar 21, 2019

The problem

On windows, if any of the ancestor folders of a monitored folder is not listable (i.e., the "List Folder" permission is unset), the file system watcher will fail. In the verbose log there are these messages:

[VK627] 15:17:59 VERBOSE: FolderWatchStateChanged events.Event{SubscriptionID:26, GlobalID:26, Time:time.Time{wall:0xbf1ce1fdcebe16cc, ext:1632000601, loc:(*time.Location)(0x1463440)}, Type:33554432, Data:map[string]interface {}{"folder":"maa2q-yxpwp", "to":"Access is denied."}}
[VK627] 15:18:05 VERBOSE: Summary for folder "maa2q-yxpwp" is map[errors:0 globalBytes:0 globalDeleted:0 globalDirectories:0 globalFiles:0 globalSymlinks:0 globalTotalItems:0 inSyncBytes:0 inSyncFiles:0 localBytes:0 localDeleted:0 localDirectories:0 localFiles:0 localSymlinks:0 localTotalItems:0 needBytes:0 needDeletes:0 needDirectories:0 needFiles:0 needSymlinks:0 needTotalItems:0 pullErrors:0 sequence:0 state:idle version:0 watchError:Access is denied.]

The "Watch Error: Access is denied" message will finally appear on the GUI webpage.

I am working in a multi-user workstation where fine-grained ACLs are configured by the administrator. It's common to have a folder structure like "C:\secret_agents\007" where I own the "007" folder but cannot list the parent folder, "secret_agent".
Older versions of Syncthing (<= 0.14.49) worked fine with such ACL settings but the above error starts to appear when I update syncthing to newer versions (>= 1.0).

Environments

  • Syncthing Version: v1.1.0
  • OS Version: Windows 10 and 7
  • File system: NTFS

Steps to reproduce

  1. Create a folder C:\parent\child and configure Syncthing to monitor it.
  2. In the access control list (ACL) of folder C:\parent , unset the "List Folder" permission and keep the "Traverse Folder" permission allowed.
  3. Enable all permissions for folder C:\parent\child.
  4. Restart Syncthing.
@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

That's how permissions work, nothing todo with syncthing. Watching requires more of them.

@gdlmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Could you elaborate to explain why the older version (<1.0) didn't require those permissions? The old watcher in version 0.14.49 has been working perfectly fine without the "List folder" permission in any ancestor directory.

@calmh

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Did the watcher actually watch or did it just not complain? If it actually did watch for changes successfully this sounds like a genuine issue, although one on a dependency of ours (the watching package).

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Perhaps @imsodin can comment, but my money is that it was caused by a change in go that broke watching roots of drives, as I don't think watching was touched for a while.

@gdlmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

According to the log message, I can pin down the error to this line of code

eventChan, err := f.Filesystem().Watch(".", ignores, ctx, f.IgnorePerms)

which probably goes down to basicfs_watch:

err = notify.Watch(filepath.Join(absName, "..."), backendChan, eventMask)

Therefore the problem might be introduced by recent modification to the syncthing/notify codes. There have been 20 commits since the release of version 0.14.49.

I don't have a development environment to further analyze the error. It would be easier for you to pin it down in a debugger.

@gdlmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@calmh The older version of syncthing did watch for changes successfully. I've been using it for 6 months. Any file modification event is usually detected in seconds although the re-scanning period is set to 1 hour.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

I guess this has some merit to it, but I suspect the ... is necessery

@imsodin

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

1. Create a folder `C:\parent\child` and configure Syncthing to monitor it.

2. In the access control list (ACL) of folder `C:\parent` , unset the "List Folder" permission and keep the "Traverse Folder" permission allowed.

3. Enable all permissions for folder `C:\parent\child`.

I understand this such, that the folder root directory is C:\parent\child and the directory without the "List folder" permission is C:\parent, i.e. a parent of the root. I don't see how that is possible, as the watcher library only operates below the folder root, i.e. should never list the contents of C:\parent. Could you please double check this.

Also if you can run a few Syncthing version between 0.14.49 and 1.0.0 to further narrow down the moment in which the behaviour changed, that would be very helpful.

@gdlmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Test setup

I've setup the folder permission using the following script

icacls C:\parent /grant:r syncuser:(X)
icacls C:\parent\child /grant:r syncuser:(OI)(CI)(M)
icacls C:\parent /inheritance:d
icacls C:\parent /remove:g Users
icacls C:\parent /remove:g "Authenticated Users"

where syncuser is the user who runs Syncthing.

Results

Version 0.14.49

File watcher works normally.

Version 0.14.50

File watcher fails to start.

Version 1.1.0

File watcher fails to start.

@gdlmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Probably commit c2ff49 accidentally introduced this problem, because it refactored lots of codes in the fs (FileSystem) package.

@imsodin

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Ah yep, I forgot about the symlink handling business - thanks for the thorough debugging!
I can't be entirely sure, but I'd bet the error comes from filepath.EvalSymlink on your root path (which was introduced in that commit, but not accidentally). That performs all kinds of magic on all parents, so I guess some of that requires the missing permission. Unfortunately I don't see a way around this, as evaluating potential symlinks is required to be able to use the paths reported by the notify library (which are evaluated too) - see #5043, which was fixed by c2ff49e.

To check my hypothesis you could run the following script: https://gist.github.com/imsodin/b1bfc8a17180404cfcc3ef4da1107c7b (go run notifyWinSym.go).

@gdlmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I've run your script and both lines fail with "access denied" error.
That confirms your theory: The error does come from EvalSymlinks.

@gdlmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

In version 0.14.49 you also called EvalSymlinks. There, if an error occurs, the root path is used instead. So the watcher can continue to work.

rootSymlinkEvaluated, err := filepath.EvalSymlinks(root)
if err != nil {
	rootSymlinkEvaluated = root
     }

Is it possible to restore this nice old behavior?

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

That's the wrong thing to do however, so I'd say no.

@imsodin

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Nope, because it wasn't nice at all, it resulted in panics: #5043

There would have to be another workaround and/or diving into filepath.EvalSymlinks to find the root cause (in the hopes that it can be circumvented). I must admit I don't see a workaround and am not much inclined to invest time into any diving.

gdlmx added a commit to gdlmx/syncthing that referenced this issue Mar 21, 2019

Possible fix for issue syncthing#5609
EvalSymlinks will fail in Windows if any of the ancestor folder is not listable. In this case we assume this folder is not symlinked.

gdlmx added a commit to gdlmx/syncthing that referenced this issue Mar 21, 2019

Fix for issue syncthing#5609
`EvalSymlinks` will fail in Windows if any of the ancestor folder is not listable. This commit implements a fallback method to get the unlinked path.

gdlmx added a commit to gdlmx/syncthing that referenced this issue Mar 21, 2019

Fix for issue syncthing#5609
`EvalSymlinks` will fail in Windows if any of the ancestor folder is not listable [1]. This commit implements a fallback method to get the unlinked path.

[1]: syncthing#5609

gdlmx added a commit to gdlmx/syncthing that referenced this issue Mar 21, 2019

Fallback EvalSymlinks method (fixes syncthing#5609)
The `filepath.EvalSymlinks` will fail in Windows
if any of the ancestor folder is not listable [1].
This commit implements a fallback method to get
the "unlinked" path (the expected result from
`EvalSymlinks`).

[1]: syncthing#5609

gdlmx added a commit to gdlmx/syncthing that referenced this issue Mar 22, 2019

Recovery from EvalSymlinks error (fixes syncthing#5609)
The `filepath.EvalSymlinks` will fail with an "access denied" error if any of the ancestor folder is not listable syncthing#5609.
This commit implements a fallback method to recover from this error if the input path does'n contain any symlink.

imsodin added a commit that referenced this issue Jun 10, 2019

@calmh calmh added this to the v1.2.0 milestone Jun 11, 2019

@calmh calmh added the bug label Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.