diff --git a/lib/model/model.go b/lib/model/model.go index f866c427c..66161d3cb 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -120,6 +120,7 @@ var ( errDevicePaused = errors.New("device is paused") errDeviceIgnored = errors.New("device is ignored") errNotRelative = errors.New("not a relative path") + errNotDir = errors.New("parent is not a directory") ) // NewModel creates and starts a new model. The model starts in read-only mode, @@ -577,7 +578,7 @@ func (m *Model) NeedSize(folder string) db.Counts { ignores := m.folderIgnores[folder] cfg := m.folderCfgs[folder] rf.WithNeedTruncated(protocol.LocalDeviceID, func(f db.FileIntf) bool { - if shouldIgnore(f, ignores, cfg.IgnoreDelete) { + if shouldIgnore(f, ignores, cfg.IgnoreDelete, defTempNamer) { return true } @@ -641,7 +642,7 @@ func (m *Model) NeedFolderFiles(folder string, page, perpage int) ([]db.FileInfo ignores := m.folderIgnores[folder] cfg := m.folderCfgs[folder] rf.WithNeedTruncated(protocol.LocalDeviceID, func(f db.FileIntf) bool { - if shouldIgnore(f, ignores, cfg.IgnoreDelete) { + if shouldIgnore(f, ignores, cfg.IgnoreDelete, defTempNamer) { return true } @@ -1113,26 +1114,22 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset return protocol.ErrNoSuchFile } - if info, err := osutil.Lstat(fn); err == nil && info.Mode()&os.ModeSymlink != 0 { - target, _, err := symlinks.Read(fn) - if err != nil { - l.Debugln("symlinks.Read:", err) - if os.IsNotExist(err) { - return protocol.ErrNoSuchFile - } - return protocol.ErrGeneric - } - if _, err := strings.NewReader(target).ReadAt(buf, offset); err != nil { - l.Debugln("symlink.Reader.ReadAt", err) - return protocol.ErrGeneric - } - return nil + if !osutil.IsDir(folderPath, filepath.Dir(name)) { + l.Debugf("%v REQ(in) for file not in dir: %s: %q / %q o=%d s=%d", m, deviceID, folder, name, offset, len(buf)) + return protocol.ErrNoSuchFile } // Only check temp files if the flag is set, and if we are set to advertise // the temp indexes. if fromTemporary && !folderCfg.DisableTempIndexes { tempFn := filepath.Join(folderPath, defTempNamer.TempName(name)) + + if info, err := osutil.Lstat(tempFn); err != nil || !info.Mode().IsRegular() { + // Reject reads for anything that doesn't exist or is something + // other than a regular file. + return protocol.ErrNoSuchFile + } + if err := readOffsetIntoBuf(tempFn, offset, buf); err == nil { return nil } @@ -1140,6 +1137,12 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset // file has finished downloading. } + if info, err := osutil.Lstat(fn); err != nil || !info.Mode().IsRegular() { + // Reject reads for anything that doesn't exist or is something + // other than a regular file. + return protocol.ErrNoSuchFile + } + err = readOffsetIntoBuf(fn, offset, buf) if os.IsNotExist(err) { return protocol.ErrNoSuchFile @@ -2537,7 +2540,7 @@ func makeForgetUpdate(files []protocol.FileInfo) []protocol.FileDownloadProgress } // shouldIgnore returns true when a file should be excluded from processing -func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool) bool { +func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool, tmpNamer tempNamer) bool { switch { case ignoreDelete && file.IsDeleted(): // ignoreDelete first because it's a very cheap test so a win if it @@ -2545,6 +2548,9 @@ func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool) // deleted files. return true + case tmpNamer.IsTemporary(file.FileName()): + return true + case ignore.IsInternal(file.FileName()): return true diff --git a/lib/model/model_test.go b/lib/model/model_test.go index c2e1f5701..17e245c84 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -227,6 +227,7 @@ type fakeConnection struct { folder string model *Model indexFn func(string, []protocol.FileInfo) + requestFn func(folder, name string, offset int64, size int, hash []byte, fromTemporary bool) ([]byte, error) mut sync.Mutex } @@ -271,6 +272,11 @@ func (f *fakeConnection) IndexUpdate(folder string, fs []protocol.FileInfo) erro } func (f *fakeConnection) Request(folder, name string, offset int64, size int, hash []byte, fromTemporary bool) ([]byte, error) { + f.mut.Lock() + defer f.mut.Unlock() + if f.requestFn != nil { + return f.requestFn(folder, name, offset, size, hash, fromTemporary) + } return f.fileData[name], nil } @@ -307,24 +313,35 @@ func (f *fakeConnection) DownloadProgress(folder string, updates []protocol.File }) } -func (f *fakeConnection) addFile(name string, flags uint32, data []byte) { +func (f *fakeConnection) addFile(name string, flags uint32, ftype protocol.FileInfoType, data []byte) { f.mut.Lock() defer f.mut.Unlock() blocks, _ := scanner.Blocks(bytes.NewReader(data), protocol.BlockSize, int64(len(data)), nil) var version protocol.Vector - version.Update(f.id.Short()) + version = version.Update(f.id.Short()) - f.files = append(f.files, protocol.FileInfo{ - Name: name, - Type: protocol.FileInfoTypeFile, - Size: int64(len(data)), - ModifiedS: time.Now().Unix(), - Permissions: flags, - Version: version, - Sequence: time.Now().UnixNano(), - Blocks: blocks, - }) + if ftype == protocol.FileInfoTypeFile || ftype == protocol.FileInfoTypeDirectory { + f.files = append(f.files, protocol.FileInfo{ + Name: name, + Type: ftype, + Size: int64(len(data)), + ModifiedS: time.Now().Unix(), + Permissions: flags, + Version: version, + Sequence: time.Now().UnixNano(), + Blocks: blocks, + }) + } else { + // Symlink + f.files = append(f.files, protocol.FileInfo{ + Name: name, + Type: ftype, + Version: version, + Sequence: time.Now().UnixNano(), + SymlinkTarget: string(data), + }) + } if f.fileData == nil { f.fileData = make(map[string][]byte) @@ -349,7 +366,7 @@ func BenchmarkRequestOut(b *testing.B) { fc := &fakeConnection{id: device1} for _, f := range files { - fc.addFile(f.Name, 0644, []byte("some data to return")) + fc.addFile(f.Name, 0644, protocol.FileInfoTypeFile, []byte("some data to return")) } m.AddConnection(fc, protocol.HelloResult{}) m.Index(device1, "default", files) diff --git a/lib/model/requests_test.go b/lib/model/requests_test.go index 331d3d6af..30b0efc6a 100644 --- a/lib/model/requests_test.go +++ b/lib/model/requests_test.go @@ -10,7 +10,10 @@ import ( "bytes" "io/ioutil" "os" + "runtime" + "strings" "testing" + "time" "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/db" @@ -42,7 +45,7 @@ func TestRequestSimple(t *testing.T) { // Send an update for the test file, wait for it to sync and be reported back. contents := []byte("test file contents\n") - fc.addFile("testfile", 0644, contents) + fc.addFile("testfile", 0644, protocol.FileInfoTypeFile, contents) fc.sendIndexUpdate() <-done @@ -57,6 +60,150 @@ func TestRequestSimple(t *testing.T) { } } +func TestSymlinkTraversalRead(t *testing.T) { + // Verify that a symlink can not be traversed for reading. + + if runtime.GOOS == "windows" { + t.Skip("no symlink support on CI") + return + } + + defer os.RemoveAll("_tmpfolder") + + m, fc := setupModelWithConnection() + defer m.Stop() + + // We listen for incoming index updates and trigger when we see one for + // the expected test file. + done := make(chan struct{}) + fc.mut.Lock() + fc.indexFn = func(folder string, fs []protocol.FileInfo) { + for _, f := range fs { + if f.Name == "symlink" { + close(done) + return + } + } + } + fc.mut.Unlock() + + // Send an update for the symlink, wait for it to sync and be reported back. + contents := []byte("..") + fc.addFile("symlink", 0644, protocol.FileInfoTypeSymlinkDirectory, contents) + fc.sendIndexUpdate() + <-done + + // Request a file by traversing the symlink + buf := make([]byte, 10) + err := m.Request(device1, "default", "symlink/requests_test.go", 0, nil, false, buf) + if err == nil || !bytes.Equal(buf, make([]byte, 10)) { + t.Error("Managed to traverse symlink") + } +} + +func TestSymlinkTraversalWrite(t *testing.T) { + // Verify that a symlink can not be traversed for writing. + + if runtime.GOOS == "windows" { + t.Skip("no symlink support on CI") + return + } + + defer os.RemoveAll("_tmpfolder") + + m, fc := setupModelWithConnection() + defer m.Stop() + + // We listen for incoming index updates and trigger when we see one for + // the expected names. + done := make(chan struct{}, 1) + badReq := make(chan string, 1) + badIdx := make(chan string, 1) + fc.mut.Lock() + fc.indexFn = func(folder string, fs []protocol.FileInfo) { + for _, f := range fs { + if f.Name == "symlink" { + done <- struct{}{} + return + } + if strings.HasPrefix(f.Name, "symlink") { + badIdx <- f.Name + return + } + } + } + fc.requestFn = func(folder, name string, offset int64, size int, hash []byte, fromTemporary bool) ([]byte, error) { + if name != "symlink" && strings.HasPrefix(name, "symlink") { + badReq <- name + } + return fc.fileData[name], nil + } + fc.mut.Unlock() + + // Send an update for the symlink, wait for it to sync and be reported back. + contents := []byte("..") + fc.addFile("symlink", 0644, protocol.FileInfoTypeSymlinkDirectory, contents) + fc.sendIndexUpdate() + <-done + + // Send an update for things behind the symlink, wait for requests for + // blocks for any of them to come back, or index entries. Hopefully none + // of that should happen. + contents = []byte("testdata testdata\n") + fc.addFile("symlink/testfile", 0644, protocol.FileInfoTypeFile, contents) + fc.addFile("symlink/testdir", 0644, protocol.FileInfoTypeDirectory, contents) + fc.addFile("symlink/testsyml", 0644, protocol.FileInfoTypeSymlinkFile, contents) + fc.sendIndexUpdate() + + select { + case name := <-badReq: + t.Fatal("Should not have requested the data for", name) + case name := <-badIdx: + t.Fatal("Should not have sent the index entry for", name) + case <-time.After(3 * time.Second): + // Unfortunately not much else to trigger on here. The puller sleep + // interval is 1s so if we didn't get any requests within two + // iterations we should be fine. + } +} + +func TestRequestCreateTmpSymlink(t *testing.T) { + // Verify that the model performs a request and creates a file based on + // an incoming index update. + + defer os.RemoveAll("_tmpfolder") + + m, fc := setupModelWithConnection() + defer m.Stop() + + // We listen for incoming index updates and trigger when we see one for + // the expected test file. + badIdx := make(chan string) + fc.mut.Lock() + fc.indexFn = func(folder string, fs []protocol.FileInfo) { + for _, f := range fs { + if f.Name == ".syncthing.testlink.tmp" { + badIdx <- f.Name + return + } + } + } + fc.mut.Unlock() + + // Send an update for the test file, wait for it to sync and be reported back. + fc.addFile(".syncthing.testlink.tmp", 0644, protocol.FileInfoTypeSymlinkDirectory, []byte("..")) + fc.sendIndexUpdate() + + select { + case name := <-badIdx: + t.Fatal("Should not have sent the index entry for", name) + case <-time.After(3 * time.Second): + // Unfortunately not much else to trigger on here. The puller sleep + // interval is 1s so if we didn't get any requests within two + // iterations we should be fine. + } +} + func setupModelWithConnection() (*Model, *fakeConnection) { cfg := defaultConfig.RawCopy() cfg.Folders[0] = config.NewFolderConfiguration("default", "_tmpfolder") diff --git a/lib/model/rwfolder.go b/lib/model/rwfolder.go index 86102190c..532e38159 100644 --- a/lib/model/rwfolder.go +++ b/lib/model/rwfolder.go @@ -382,23 +382,81 @@ func (f *rwFolder) pullerIteration(ignores *ignore.Matcher) int { folderFiles := f.model.folderFiles[f.folderID] f.model.fmut.RUnlock() - // !!! - // WithNeed takes a database snapshot (by necessity). By the time we've - // handled a bunch of files it might have become out of date and we might - // be attempting to sync with an old version of a file... - // !!! - changed := 0 + var processDirectly []protocol.FileInfo + + // Iterate the list of items that we need and sort them into piles. + // Regular files to pull goes into the file queue, everything else + // (directories, symlinks and deletes) goes into the "process directly" + // pile. + + folderFiles.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool { + if shouldIgnore(intf, ignores, f.ignoreDelete, defTempNamer) { + return true + } + + if err := fileValid(intf); err != nil { + // The file isn't valid so we can't process it. Pretend that we + // tried and set the error for the file. + f.newError(intf.FileName(), err) + changed++ + return true + } + + file := intf.(protocol.FileInfo) + + switch { + case file.IsDeleted(): + processDirectly = append(processDirectly, file) + changed++ + + case file.Type == protocol.FileInfoTypeFile: + // Queue files for processing after directories and symlinks, if + // it has availability. + + devices := folderFiles.Availability(file.Name) + for _, dev := range devices { + if f.model.ConnectedTo(dev) { + f.queue.Push(file.Name, file.Size, file.ModTime()) + changed++ + break + } + } + + default: + // Directories, symlinks + processDirectly = append(processDirectly, file) + changed++ + } + + return true + }) + + // Sort the "process directly" pile by number of path components. This + // ensures that we handle parents before children. + + sort.Sort(byComponentCount(processDirectly)) + + // Process the list. fileDeletions := map[string]protocol.FileInfo{} dirDeletions := []protocol.FileInfo{} buckets := map[string][]protocol.FileInfo{} - handleItem := func(fi protocol.FileInfo) bool { + for _, fi := range processDirectly { + // 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(fi.Name)) { + f.newError(fi.Name, errNotDir) + continue + } + switch { case fi.IsDeleted(): // A deleted file, directory or symlink if fi.IsDirectory() { + // Perform directory deletions at the end, as we may have + // files to delete inside them before we get to that point. dirDeletions = append(dirDeletions, fi) } else { fileDeletions[fi.Name] = fi @@ -413,63 +471,22 @@ func (f *rwFolder) pullerIteration(ignores *ignore.Matcher) int { buckets[key] = append(buckets[key], df) } } - changed++ case fi.IsDirectory() && !fi.IsSymlink(): l.Debugln("Handling directory", fi.Name) f.handleDir(fi) - changed++ case fi.IsSymlink(): l.Debugln("Handling symlink", fi.Name) f.handleSymlink(fi) - changed++ default: - return false + l.Warnln(fi) + panic("unhandleable item type, can't happen") } - return true } - folderFiles.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool { - // Needed items are delivered sorted lexicographically. We'll handle - // directories as they come along, so parents before children. Files - // are queued and the order may be changed later. - - if shouldIgnore(intf, ignores, f.ignoreDelete) { - return true - } - - if err := fileValid(intf); err != nil { - // The file isn't valid so we can't process it. Pretend that we - // tried and set the error for the file. - f.newError(intf.FileName(), err) - changed++ - return true - } - - file := intf.(protocol.FileInfo) - l.Debugln(f, "handling", file.Name) - if !handleItem(file) { - // A new or changed file or symlink. This is the only case where - // we do stuff concurrently in the background. We only queue - // files where we are connected to at least one device that has - // the file. - - devices := folderFiles.Availability(file.Name) - for _, dev := range devices { - if f.model.ConnectedTo(dev) { - f.queue.Push(file.Name, file.Size, file.ModTime()) - changed++ - break - } - } - } - - return true - }) - - // Reorder the file queue according to configuration + // Now do the file queue. Reorder it according to configuration. switch f.order { case config.OrderRandom: @@ -486,7 +503,7 @@ func (f *rwFolder) pullerIteration(ignores *ignore.Matcher) int { f.queue.SortNewestFirst() } - // Process the file queue + // Process the file queue. nextFile: for { @@ -509,10 +526,17 @@ nextFile: continue } - // Handles races where an index update arrives changing what the file - // is between queueing and retrieving it from the queue, effectively - // changing how the file should be handled. - if handleItem(fi) { + if fi.IsDeleted() || fi.Type != protocol.FileInfoTypeFile { + // The item has changed type or status in the index while we + // were processing directories above. + f.queue.Done(fileName) + continue + } + + // 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(fi.Name)) { + f.newError(fi.Name, errNotDir) continue } @@ -779,6 +803,7 @@ func (f *rwFolder) deleteDir(file protocol.FileInfo, matcher *ignore.Matcher) { f.newError(file.Name, err) return } + // Delete any temporary files lying around in the directory dir, _ := os.Open(realName) if dir != nil { @@ -1727,3 +1752,29 @@ func windowsInvalidFilename(name string) bool { // The path must not contain any disallowed characters return strings.ContainsAny(name, windowsDisallowedCharacters) } + +// byComponentCount sorts by the number of path components in Name, that is +// "x/y" sorts before "foo/bar/baz". +type byComponentCount []protocol.FileInfo + +func (l byComponentCount) Len() int { + return len(l) +} + +func (l byComponentCount) Less(a, b int) bool { + return componentCount(l[a].Name) < componentCount(l[b].Name) +} + +func (l byComponentCount) Swap(a, b int) { + l[a], l[b] = l[b], l[a] +} + +func componentCount(name string) int { + count := 0 + for _, codepoint := range name { + if codepoint == os.PathSeparator { + count++ + } + } + return count +} diff --git a/lib/osutil/isdir.go b/lib/osutil/isdir.go new file mode 100644 index 000000000..5f39af930 --- /dev/null +++ b/lib/osutil/isdir.go @@ -0,0 +1,49 @@ +// Copyright (C) 2016 The Syncthing Authors. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +package osutil + +import ( + "os" + "path/filepath" + "strings" +) + +// IsDir returns true if base and every path component of name up to and +// including filepath.Join(base, name) is a directory (and not a symlink or +// similar). Base and name must both be clean and name must be relative to +// base. +func IsDir(base, name string) bool { + path := base + info, err := Lstat(path) + if err != nil { + return false + } + if !info.IsDir() { + return false + } + + if name == "." { + // The result of calling IsDir("some/where", filepath.Dir("foo")) + return true + } + + parts := strings.Split(name, string(os.PathSeparator)) + for _, part := range parts { + path = filepath.Join(path, part) + info, err := Lstat(path) + if err != nil { + return false + } + if info.Mode()&os.ModeSymlink != 0 { + return false + } + if !info.IsDir() { + return false + } + } + return true +} diff --git a/lib/osutil/isdir_test.go b/lib/osutil/isdir_test.go new file mode 100644 index 000000000..f155e4cf7 --- /dev/null +++ b/lib/osutil/isdir_test.go @@ -0,0 +1,75 @@ +// Copyright (C) 2016 The Syncthing Authors. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +package osutil_test + +import ( + "os" + "testing" + + "github.com/syncthing/syncthing/lib/osutil" + "github.com/syncthing/syncthing/lib/symlinks" +) + +func TestIsDir(t *testing.T) { + if !symlinks.Supported { + t.Skip("pointless test") + return + } + + os.RemoveAll("testdata") + defer os.RemoveAll("testdata") + os.MkdirAll("testdata/a/b/c", 0755) + symlinks.Create("testdata/a/l", "b", symlinks.TargetDirectory) + + // a/l -> b, so a/l/c should resolve by normal stat + info, err := osutil.Lstat("testdata/a/l/c") + if err != nil { + t.Fatal("unexpected error", err) + } + if !info.IsDir() { + t.Fatal("error in setup, a/l/c should be a directory") + } + + cases := []struct { + name string + isDir bool + }{ + // Exist + {".", true}, + {"a", true}, + {"a/b", true}, + {"a/b/c", true}, + // Don't exist + {"x", false}, + {"a/x", false}, + {"a/b/x", false}, + {"a/x/c", false}, + // Symlink or behind symlink + {"a/l", false}, + {"a/l/c", false}, + } + + for _, tc := range cases { + if res := osutil.IsDir("testdata", tc.name); res != tc.isDir { + t.Errorf("IsDir(%q) = %v, should be %v", tc.name, res, tc.isDir) + } + } +} + +var isDirResult bool + +func BenchmarkIsDir(b *testing.B) { + os.RemoveAll("testdata") + defer os.RemoveAll("testdata") + os.MkdirAll("testdata/a/b/c", 0755) + + for i := 0; i < b.N; i++ { + isDirResult = osutil.IsDir("testdata", "a/b/c") + } + + b.ReportAllocs() +}