From beda37f28b0932c4d5861edd80227c90c60a1df4 Mon Sep 17 00:00:00 2001 From: pullmerge <166967364+pullmerge@users.noreply.github.com> Date: Fri, 23 May 2025 18:36:06 +0800 Subject: [PATCH 1/3] refactor: use slices.Contains to simplify code (#10121) There is a [new function](https://pkg.go.dev/slices@go1.21.0#Contains) added in the go1.21 standard library, which can make the code more concise and easy to read. --- lib/model/folder_test.go | 14 +++----------- lib/model/model.go | 9 ++++----- lib/model/model_test.go | 13 +++++-------- 3 files changed, 12 insertions(+), 24 deletions(-) diff --git a/lib/model/folder_test.go b/lib/model/folder_test.go index a09b5af30..f73c665ff 100644 --- a/lib/model/folder_test.go +++ b/lib/model/folder_test.go @@ -8,6 +8,7 @@ package model import ( "path/filepath" + "slices" "testing" "github.com/d4l3k/messagediff" @@ -117,20 +118,11 @@ func unifySubsCases() []unifySubsCase { return cases } -func unifyExists(f string, tc unifySubsCase) bool { - for _, e := range tc.exists { - if f == e { - return true - } - } - return false -} - func TestUnifySubs(t *testing.T) { cases := unifySubsCases() for i, tc := range cases { exists := func(f string) bool { - return unifyExists(f, tc) + return slices.Contains(tc.exists, f) } out := unifySubs(tc.in, exists) if diff, equal := messagediff.PrettyDiff(tc.out, out); !equal { @@ -146,7 +138,7 @@ func BenchmarkUnifySubs(b *testing.B) { for i := 0; i < b.N; i++ { for _, tc := range cases { exists := func(f string) bool { - return unifyExists(f, tc) + return slices.Contains(tc.exists, f) } unifySubs(tc.in, exists) } diff --git a/lib/model/model.go b/lib/model/model.go index 9e898395d..34a99c7f8 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -21,6 +21,7 @@ import ( "path/filepath" "reflect" "runtime" + "slices" "strings" stdsync "sync" "sync/atomic" @@ -1804,11 +1805,9 @@ func (m *model) handleAutoAccepts(deviceID protocol.DeviceID, folder protocol.Fo l.Infof("Failed to auto-accept folder %s from %s due to path conflict", folder.Description(), deviceID) return config.FolderConfiguration{}, false } else { - for _, device := range cfg.DeviceIDs() { - if device == deviceID { - // Already shared nothing todo. - return config.FolderConfiguration{}, false - } + if slices.Contains(cfg.DeviceIDs(), deviceID) { + // Already shared nothing todo. + return config.FolderConfiguration{}, false } if cfg.Type == config.FolderTypeReceiveEncrypted { if len(ccDeviceInfos.remote.EncryptionPasswordToken) == 0 && len(ccDeviceInfos.local.EncryptionPasswordToken) == 0 { diff --git a/lib/model/model_test.go b/lib/model/model_test.go index ebffee28d..f7399e4d9 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -17,6 +17,7 @@ import ( "os" "path/filepath" "runtime/pprof" + "slices" "sort" "strconv" "strings" @@ -1253,10 +1254,8 @@ func TestAutoAcceptPausedWhenFolderConfigChanged(t *testing.T) { } else if fcfg.Path != idOther { t.Error("folder path changed") } else { - for _, dev := range fcfg.DeviceIDs() { - if dev == device1 { - return - } + if slices.Contains(fcfg.DeviceIDs(), device1) { + return } t.Error("device missing") } @@ -1302,10 +1301,8 @@ func TestAutoAcceptPausedWhenFolderConfigNotChanged(t *testing.T) { } else if fcfg.Path != idOther { t.Error("folder path changed") } else { - for _, dev := range fcfg.DeviceIDs() { - if dev == device1 { - return - } + if slices.Contains(fcfg.DeviceIDs(), device1) { + return } t.Error("device missing") } From 1a131a56f277d138b7792017a1900cf73ef58126 Mon Sep 17 00:00:00 2001 From: Ashish Bhate Date: Sat, 24 May 2025 11:05:32 +0530 Subject: [PATCH 2/3] fix(versioner): fix perms of created folders (fixes #9626) (#10105) As suggested in the linked issue, I've updated the versioner code to use the permissions of the corresponding directory in the synced folder, when creating the folder in the versions directory ### Testing - Some tests are included with the PR. Happy to add more if you think there are some edge-cases that we're missing. - I've tested manually on linux to confirm the permissions of the created directories. - I haven't tested on Windows or OSX (I don't have access to these OS) --- lib/versioner/simple_test.go | 100 +++++++++++++++++++++++++++++++++++ lib/versioner/util.go | 50 ++++++++++++++++-- 2 files changed, 147 insertions(+), 3 deletions(-) diff --git a/lib/versioner/simple_test.go b/lib/versioner/simple_test.go index 0087f826b..0bdbeeaff 100644 --- a/lib/versioner/simple_test.go +++ b/lib/versioner/simple_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/syncthing/syncthing/lib/build" "github.com/syncthing/syncthing/lib/config" ) @@ -156,3 +157,102 @@ func TestPathTildes(t *testing.T) { t.Fatalf("found versioned file %q, want one that begins with %q", got, testPath) } } + +func TestArchiveFoldersCreationPermission(t *testing.T) { + if build.IsWindows { + t.Skip("Skipping on Windows") + return + } + dir := t.TempDir() + versionsDir := t.TempDir() + + cfg := config.FolderConfiguration{ + FilesystemType: config.FilesystemTypeBasic, + Path: dir, + Versioning: config.VersioningConfiguration{ + FSPath: versionsDir, + FSType: config.FilesystemTypeBasic, + Params: map[string]string{ + "keep": "2", + }, + }, + } + vfs := cfg.Filesystem(nil) + v := newSimple(cfg) + + // Create two folders and set their permissions + folder1Path := filepath.Join(dir, "folder1") + folder1Perms := os.FileMode(0o777) + folder1VersionsPath := filepath.Join(versionsDir, "folder1") + err := os.Mkdir(folder1Path, folder1Perms) + if err != nil { + t.Fatal(err) + } + // chmod incase umask changes the create permissions + err = os.Chmod(folder1Path, folder1Perms) + if err != nil { + t.Fatal(err) + } + + folder2Path := filepath.Join(folder1Path, "földer2") + folder2VersionsPath := filepath.Join(folder1VersionsPath, "földer2") + folder2Perms := os.FileMode(0o744) + err = os.Mkdir(folder2Path, folder2Perms) + if err != nil { + t.Fatal(err) + } + // chmod incase umask changes the create permissions + err = os.Chmod(folder2Path, folder2Perms) + if err != nil { + t.Fatal(err) + } + + // create a file + filePath := filepath.Join("folder1", "földer2", "testFile") + f, err := vfs.Create(filePath) + if err != nil { + t.Fatal(err) + } + f.Close() + + if err := v.Archive(filePath); err != nil { + t.Error(err) + } + + // check permissions of the created version folders + folder1VersionsInfo, err := os.Stat(folder1VersionsPath) + if err != nil { + t.Fatal(err) + } + if folder1VersionsInfo.Mode().Perm() != folder1Perms { + t.Errorf("folder1 permissions %v, want %v", folder1VersionsInfo.Mode(), folder1Perms) + } + + folder2VersionsInfo, err := os.Stat(folder2VersionsPath) + if err != nil { + t.Fatal(err) + } + if folder2VersionsInfo.Mode().Perm() != folder2Perms { + t.Errorf("földer2 permissions %v, want %v", folder2VersionsInfo.Mode(), folder2Perms) + } + + // Archive again to test that archiving doesn't fail if the versioned folders already exist + if err := v.Archive(filePath); err != nil { + t.Error(err) + } + folder1VersionsInfo, err = os.Stat(folder1VersionsPath) + if err != nil { + t.Fatal(err) + } + if folder1VersionsInfo.Mode().Perm() != folder1Perms { + t.Errorf("folder1 permissions %v, want %v", folder1VersionsInfo.Mode(), folder1Perms) + } + + folder2VersionsInfo, err = os.Stat(folder2VersionsPath) + if err != nil { + t.Fatal(err) + } + if folder2VersionsInfo.Mode().Perm() != folder2Perms { + t.Errorf("földer2 permissions %v, want %v", folder2VersionsInfo.Mode(), folder2Perms) + } +} diff --git a/lib/versioner/util.go b/lib/versioner/util.go index 4693d1d9c..66d8c90ba 100644 --- a/lib/versioner/util.go +++ b/lib/versioner/util.go @@ -10,6 +10,7 @@ import ( "context" "errors" "fmt" + "os" "path/filepath" "regexp" "sort" @@ -164,9 +165,8 @@ func archiveFile(method fs.CopyRangeMethod, srcFs, dstFs fs.Filesystem, filePath file := filepath.Base(filePath) inFolderPath := filepath.Dir(filePath) - - err = dstFs.MkdirAll(inFolderPath, 0o755) - if err != nil && !fs.IsExist(err) { + err = dupDirTree(srcFs, dstFs, inFolderPath) + if err != nil { l.Debugln("archiving", filePath, err) return err } @@ -190,6 +190,50 @@ func archiveFile(method fs.CopyRangeMethod, srcFs, dstFs fs.Filesystem, filePath return err } +func dupDirTree(srcFs, dstFs fs.Filesystem, folderPath string) error { + // Return early if the folder already exists. + _, err := dstFs.Stat(folderPath) + if err == nil || !fs.IsNotExist(err) { + return err + } + hadParent := true + for i := range folderPath { + if os.IsPathSeparator(folderPath[i]) { + // If the parent folder didn't exist, then this folder doesn't exist + // so we can skip the check + if hadParent { + _, err := dstFs.Stat(folderPath[:i]) + if err == nil { + continue + } + if !fs.IsNotExist(err) { + return err + } + } + hadParent = false + err := dupDirWithPerms(srcFs, dstFs, folderPath[:i]) + if err != nil { + return err + } + } + } + return dupDirWithPerms(srcFs, dstFs, folderPath) +} + +func dupDirWithPerms(srcFs, dstFs fs.Filesystem, folderPath string) error { + srcStat, err := srcFs.Stat(folderPath) + if err != nil { + return err + } + // If we call Mkdir with srcStat.Mode(), we won't get the expected perms because of umask + // So, we create the folder with 0700, and then change the perms to the srcStat.Mode() + err = dstFs.Mkdir(folderPath, 0o700) + if err != nil { + return err + } + return dstFs.Chmod(folderPath, srcStat.Mode()) +} + func restoreFile(method fs.CopyRangeMethod, src, dst fs.Filesystem, filePath string, versionTime time.Time, tagger fileTagger) error { tag := versionTime.In(time.Local).Truncate(time.Second).Format(TimeFormat) taggedFilePath := tagger(filePath, tag) From 64b5a1b738d38fc7c232a03daeca4f20dbd8dc8d Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sun, 25 May 2025 08:10:17 +0200 Subject: [PATCH 3/3] fix(syncthing): ensure both config and data dirs exist at startup (fixes #10126) (#10127) Previously we'd only ensure the config dir, which is often but not always the same as the data dir. Fixes #10126 --- cmd/syncthing/main.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 49e3bbc8f..6a1185d98 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -349,10 +349,12 @@ func (options serveOptions) Run() error { return nil } - // Ensure that our home directory exists. - if err := syncthing.EnsureDir(locations.GetBaseDir(locations.ConfigBaseDir), 0o700); err != nil { - l.Warnln("Failure on home directory:", err) - os.Exit(svcutil.ExitError.AsInt()) + // Ensure that our config and data directories exist. + for _, loc := range []locations.BaseDirEnum{locations.ConfigBaseDir, locations.DataBaseDir} { + if err := syncthing.EnsureDir(locations.GetBaseDir(loc), 0o700); err != nil { + l.Warnln("Failed to ensure directory exists:", err) + os.Exit(svcutil.ExitError.AsInt()) + } } if options.UpgradeTo != "" {