From 35b699dc77a23df3511940fb58d78a47308259fe Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Sun, 22 Sep 2019 09:03:22 +0200 Subject: [PATCH] lib/fs: Check events against both the user and eval root (#6013) --- lib/fs/basicfs.go | 4 ++-- lib/fs/basicfs_unix.go | 24 +++++++++++++----------- lib/fs/basicfs_watch.go | 10 +++++----- lib/fs/basicfs_watch_test.go | 10 +++++----- lib/fs/basicfs_windows.go | 34 +++++++++++++++++++++------------- lib/fs/basicfs_windows_test.go | 17 ++++++++++++++++- 6 files changed, 62 insertions(+), 37 deletions(-) diff --git a/lib/fs/basicfs.go b/lib/fs/basicfs.go index bd90bf7f0..fb9087431 100644 --- a/lib/fs/basicfs.go +++ b/lib/fs/basicfs.go @@ -345,6 +345,6 @@ func (e *ErrWatchEventOutsideRoot) Error() string { return e.msg } -func (f *BasicFilesystem) newErrWatchEventOutsideRoot(absPath, root string) *ErrWatchEventOutsideRoot { - return &ErrWatchEventOutsideRoot{fmt.Sprintf("Watching for changes encountered an event outside of the filesystem root: f.root==%v, root==%v, path==%v. This should never happen, please report this message to forum.syncthing.net.", f.root, root, absPath)} +func (f *BasicFilesystem) newErrWatchEventOutsideRoot(absPath string, roots []string) *ErrWatchEventOutsideRoot { + return &ErrWatchEventOutsideRoot{fmt.Sprintf("Watching for changes encountered an event outside of the filesystem root: f.root==%v, roots==%v, path==%v. This should never happen, please report this message to forum.syncthing.net.", f.root, roots, absPath)} } diff --git a/lib/fs/basicfs_unix.go b/lib/fs/basicfs_unix.go index 9c4477334..eb2912cca 100644 --- a/lib/fs/basicfs_unix.go +++ b/lib/fs/basicfs_unix.go @@ -60,14 +60,16 @@ func (f *BasicFilesystem) Roots() ([]string, error) { // unrooted) or an error if the given path is not a subpath and handles the // special case when the given path is the folder root without a trailing // pathseparator. -func (f *BasicFilesystem) unrootedChecked(absPath, root string) (string, *ErrWatchEventOutsideRoot) { - if absPath+string(PathSeparator) == root { - return ".", nil +func (f *BasicFilesystem) unrootedChecked(absPath string, roots []string) (string, *ErrWatchEventOutsideRoot) { + for _, root := range roots { + if absPath+string(PathSeparator) == root { + return ".", nil + } + if strings.HasPrefix(absPath, root) { + return rel(absPath, root), nil + } } - if !strings.HasPrefix(absPath, root) { - return "", f.newErrWatchEventOutsideRoot(absPath, root) - } - return rel(absPath, root), nil + return "", f.newErrWatchEventOutsideRoot(absPath, roots) } func rel(path, prefix string) string { @@ -78,16 +80,16 @@ var evalSymlinks = filepath.EvalSymlinks // watchPaths adjust the folder root for use with the notify backend and the // corresponding absolute path to be passed to notify to watch name. -func (f *BasicFilesystem) watchPaths(name string) (string, string, error) { +func (f *BasicFilesystem) watchPaths(name string) (string, []string, error) { root, err := evalSymlinks(f.root) if err != nil { - return "", "", err + return "", nil, err } absName, err := rooted(name, root) if err != nil { - return "", "", err + return "", nil, err } - return filepath.Join(absName, "..."), root, nil + return filepath.Join(absName, "..."), []string{root}, nil } diff --git a/lib/fs/basicfs_watch.go b/lib/fs/basicfs_watch.go index d89d6904b..903123bce 100644 --- a/lib/fs/basicfs_watch.go +++ b/lib/fs/basicfs_watch.go @@ -21,7 +21,7 @@ import ( var backendBuffer = 500 func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context, ignorePerms bool) (<-chan Event, <-chan error, error) { - watchPath, root, err := f.watchPaths(name) + watchPath, roots, err := f.watchPaths(name) if err != nil { return nil, nil, err } @@ -36,7 +36,7 @@ func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context if ignore.SkipIgnoredDirs() { absShouldIgnore := func(absPath string) bool { - rel, err := f.unrootedChecked(absPath, root) + rel, err := f.unrootedChecked(absPath, roots) if err != nil { return true } @@ -55,12 +55,12 @@ func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context } errChan := make(chan error) - go f.watchLoop(name, root, backendChan, outChan, errChan, ignore, ctx) + go f.watchLoop(name, roots, backendChan, outChan, errChan, ignore, ctx) return outChan, errChan, nil } -func (f *BasicFilesystem) watchLoop(name, evalRoot string, backendChan chan notify.EventInfo, outChan chan<- Event, errChan chan<- error, ignore Matcher, ctx context.Context) { +func (f *BasicFilesystem) watchLoop(name string, roots []string, backendChan chan notify.EventInfo, outChan chan<- Event, errChan chan<- error, ignore Matcher, ctx context.Context) { for { // Detect channel overflow if len(backendChan) == backendBuffer { @@ -79,7 +79,7 @@ func (f *BasicFilesystem) watchLoop(name, evalRoot string, backendChan chan noti select { case ev := <-backendChan: - relPath, err := f.unrootedChecked(ev.Path(), evalRoot) + relPath, err := f.unrootedChecked(ev.Path(), roots) if err != nil { select { case errChan <- err: diff --git a/lib/fs/basicfs_watch_test.go b/lib/fs/basicfs_watch_test.go index 1a1b40f01..72eb0ae03 100644 --- a/lib/fs/basicfs_watch_test.go +++ b/lib/fs/basicfs_watch_test.go @@ -166,7 +166,7 @@ func TestWatchWinRoot(t *testing.T) { // testFs is Filesystem, but we need BasicFilesystem here root := `D:\` fs := newBasicFilesystem(root) - watch, root, err := fs.watchPaths(".") + watch, roots, err := fs.watchPaths(".") if err != nil { t.Fatal(err) } @@ -178,7 +178,7 @@ func TestWatchWinRoot(t *testing.T) { } cancel() }() - fs.watchLoop(".", root, backendChan, outChan, errChan, fakeMatcher{}, ctx) + fs.watchLoop(".", roots, backendChan, outChan, errChan, fakeMatcher{}, ctx) }() // filepath.Dir as watch has a /... suffix @@ -210,7 +210,7 @@ func TestWatchOutside(t *testing.T) { // testFs is Filesystem, but we need BasicFilesystem here fs := newBasicFilesystem(testDirAbs) - go fs.watchLoop(".", testDirAbs, backendChan, outChan, errChan, fakeMatcher{}, ctx) + go fs.watchLoop(".", []string{testDirAbs}, backendChan, outChan, errChan, fakeMatcher{}, ctx) backendChan <- fakeEventInfo(filepath.Join(filepath.Dir(testDirAbs), "outside")) @@ -234,7 +234,7 @@ func TestWatchSubpath(t *testing.T) { fs := newBasicFilesystem(testDirAbs) abs, _ := fs.rooted("sub") - go fs.watchLoop("sub", testDirAbs, backendChan, outChan, errChan, fakeMatcher{}, ctx) + go fs.watchLoop("sub", []string{testDirAbs}, backendChan, outChan, errChan, fakeMatcher{}, ctx) backendChan <- fakeEventInfo(filepath.Join(abs, "file")) @@ -347,7 +347,7 @@ func TestWatchSymlinkedRoot(t *testing.T) { func TestUnrootedChecked(t *testing.T) { fs := newBasicFilesystem(testDirAbs) - if unrooted, err := fs.unrootedChecked("/random/other/path", testDirAbs); err == nil { + if unrooted, err := fs.unrootedChecked("/random/other/path", []string{testDirAbs}); err == nil { t.Error("unrootedChecked did not return an error on outside path, but returned", unrooted) } } diff --git a/lib/fs/basicfs_windows.go b/lib/fs/basicfs_windows.go index cedf917d4..ba2519881 100644 --- a/lib/fs/basicfs_windows.go +++ b/lib/fs/basicfs_windows.go @@ -156,17 +156,19 @@ func (f *BasicFilesystem) Roots() ([]string, error) { // unrooted) or an error if the given path is not a subpath and handles the // special case when the given path is the folder root without a trailing // pathseparator. -func (f *BasicFilesystem) unrootedChecked(absPath, root string) (string, error) { +func (f *BasicFilesystem) unrootedChecked(absPath string, roots []string) (string, error) { absPath = f.resolveWin83(absPath) lowerAbsPath := UnicodeLowercase(absPath) - lowerRoot := UnicodeLowercase(root) - if lowerAbsPath+string(PathSeparator) == lowerRoot { - return ".", nil + for _, root := range roots { + lowerRoot := UnicodeLowercase(root) + if lowerAbsPath+string(PathSeparator) == lowerRoot { + return ".", nil + } + if strings.HasPrefix(lowerAbsPath, lowerRoot) { + return rel(absPath, root), nil + } } - if !strings.HasPrefix(lowerAbsPath, lowerRoot) { - return "", f.newErrWatchEventOutsideRoot(lowerAbsPath, lowerRoot) - } - return rel(absPath, root), nil + return "", f.newErrWatchEventOutsideRoot(lowerAbsPath, roots) } func rel(path, prefix string) string { @@ -294,10 +296,10 @@ func evalSymlinks(in string) (string, error) { // watchPaths adjust the folder root for use with the notify backend and the // corresponding absolute path to be passed to notify to watch name. -func (f *BasicFilesystem) watchPaths(name string) (string, string, error) { +func (f *BasicFilesystem) watchPaths(name string) (string, []string, error) { root, err := evalSymlinks(f.root) if err != nil { - return "", "", err + return "", nil, err } // Remove `\\?\` prefix if the path is just a drive letter as a dirty @@ -308,11 +310,17 @@ func (f *BasicFilesystem) watchPaths(name string) (string, string, error) { absName, err := rooted(name, root) if err != nil { - return "", "", err + return "", nil, err } - root = f.resolveWin83(root) + roots := []string{f.resolveWin83(root)} absName = f.resolveWin83(absName) - return filepath.Join(absName, "..."), root, nil + // Events returned from fs watching are all over the place, so allow + // both the user's input and the result of "canonicalizing" the path. + if roots[0] != f.root { + roots = append(roots, f.root) + } + + return filepath.Join(absName, "..."), roots, nil } diff --git a/lib/fs/basicfs_windows_test.go b/lib/fs/basicfs_windows_test.go index 915b2c28e..6305377f4 100644 --- a/lib/fs/basicfs_windows_test.go +++ b/lib/fs/basicfs_windows_test.go @@ -131,7 +131,7 @@ func TestRelUnrootedCheckedWindows(t *testing.T) { // on these test cases. for _, root := range []string{tc.root, strings.ToLower(tc.root), strings.ToUpper(tc.root)} { fs := BasicFilesystem{root: root} - if res, err := fs.unrootedChecked(tc.abs, tc.root); err != nil { + if res, err := fs.unrootedChecked(tc.abs, []string{tc.root}); err != nil { t.Errorf(`Unexpected error from unrootedChecked("%v", "%v"): %v (fs.root: %v)`, tc.abs, tc.root, err, root) } else if res != tc.expectedRel { t.Errorf(`unrootedChecked("%v", "%v") == "%v", expected "%v" (fs.root: %v)`, tc.abs, tc.root, res, tc.expectedRel, root) @@ -140,6 +140,21 @@ func TestRelUnrootedCheckedWindows(t *testing.T) { } } +// TestMultipleRoot checks that fs.unrootedChecked returns the correct path +// when given more than one possible root path. +func TestMultipleRoot(t *testing.T) { + root := `c:\foO` + roots := []string{root, `d:\`} + rel := `bar` + path := filepath.Join(root, rel) + fs := BasicFilesystem{root: root} + if res, err := fs.unrootedChecked(path, roots); err != nil { + t.Errorf(`Unexpected error from unrootedChecked("%v", "%v"): %v (fs.root: %v)`, path, roots, err, root) + } else if res != rel { + t.Errorf(`unrootedChecked("%v", "%v") == "%v", expected "%v" (fs.root: %v)`, path, roots, res, rel, root) + } +} + func TestGetFinalPath(t *testing.T) { testCases := []struct { input string