diff --git a/internal/model/puller.go b/internal/model/puller.go index d1701246f..66adf801a 100644 --- a/internal/model/puller.go +++ b/internal/model/puller.go @@ -272,25 +272,50 @@ func (p *Puller) handleDir(file protocol.FileInfo) { l.Debugf("need dir\n\t%v\n\t%v", file, curFile) } - var err error - if info, err := os.Stat(realName); err != nil && os.IsNotExist(err) { - err = os.MkdirAll(realName, mode) + if info, err := os.Stat(realName); err != nil { + if os.IsNotExist(err) { + // The directory doesn't exist, so we create it with the right + // mode bits from the start. + + mkdir := func(path string) error { + // We declare a function that acts on only the path name, so + // we can pass it to InWritableDir. We use a regular Mkdir and + // not MkdirAll because the parent should already exist. + return os.Mkdir(path, mode) + } + + if err = osutil.InWritableDir(mkdir, realName); err == nil { + p.model.updateLocal(p.repo, file) + } else { + l.Infof("Puller (repo %q, file %q): %v", p.repo, file.Name, err) + } + return + } + + // Weird error when stat()'ing the dir. Probably won't work to do + // anything else with it if we can't even stat() it. + l.Infof("Puller (repo %q, file %q): %v", p.repo, file.Name, err) + return } else if !info.IsDir() { l.Infof("Puller (repo %q, file %q): should be dir, but is not", p.repo, file.Name) return - } else { - err = os.Chmod(realName, mode) } - if err == nil { + // The directory already exists, so we just correct the mode bits. (We + // don't handle modification times on directories, because that sucks...) + // It's OK to change mode bits on stuff within non-writable directories. + + if err := os.Chmod(realName, mode); err == nil { p.model.updateLocal(p.repo, file) + } else { + l.Infof("Puller (repo %q, file %q): %v", p.repo, file.Name, err) } } // deleteDir attempts to delete the given directory func (p *Puller) deleteDir(file protocol.FileInfo) { realName := filepath.Join(p.dir, file.Name) - err := os.Remove(realName) + err := osutil.InWritableDir(os.Remove, realName) if err == nil || os.IsNotExist(err) { p.model.updateLocal(p.repo, file) } @@ -299,27 +324,12 @@ func (p *Puller) deleteDir(file protocol.FileInfo) { // deleteFile attempts to delete the given file func (p *Puller) deleteFile(file protocol.FileInfo) { realName := filepath.Join(p.dir, file.Name) - realDir := filepath.Dir(realName) - if info, err := os.Stat(realDir); err == nil && info.IsDir() && info.Mode()&04 == 0 { - // A non-writeable directory (for this user; we assume that's the - // relevant part). Temporarily change the mode so we can delete the - // file inside it. - err = os.Chmod(realDir, 0755) - if err == nil { - defer func() { - err = os.Chmod(realDir, info.Mode()) - if err != nil { - panic(err) - } - }() - } - } var err error if p.versioner != nil { - err = p.versioner.Archive(realName) + err = osutil.InWritableDir(p.versioner.Archive, realName) } else { - err = os.Remove(realName) + err = osutil.InWritableDir(os.Remove, realName) } if err != nil { diff --git a/internal/model/sharedpullerstate.go b/internal/model/sharedpullerstate.go index e2dfafb51..ca5ecefe5 100644 --- a/internal/model/sharedpullerstate.go +++ b/internal/model/sharedpullerstate.go @@ -5,7 +5,6 @@ package model import ( - "fmt" "os" "path/filepath" "sync" @@ -47,21 +46,13 @@ func (s *sharedPullerState) tempFile() (*os.File, error) { return s.fd, nil } - // Ensure that the parent directory exists or can be created + // Ensure that the parent directory is writable. This is + // osutil.InWritableDir except we need to do more stuff so we duplicate it + // here. dir := filepath.Dir(s.tempName) - if info, err := os.Stat(dir); err != nil && os.IsNotExist(err) { - err = os.MkdirAll(dir, 0755) - if err != nil { - s.earlyCloseLocked("dst mkdir", err) - return nil, err - } - } else if err != nil { + if info, err := os.Stat(dir); err != nil { s.earlyCloseLocked("dst stat dir", err) return nil, err - } else if !info.IsDir() { - err = fmt.Errorf("%q: not a directory", dir) - s.earlyCloseLocked("dst mkdir", err) - return nil, err } else if info.Mode()&04 == 0 { err := os.Chmod(dir, 0755) if err == nil { diff --git a/internal/osutil/osutil.go b/internal/osutil/osutil.go index 32f5595ed..f73bd2ed9 100644 --- a/internal/osutil/osutil.go +++ b/internal/osutil/osutil.go @@ -45,3 +45,28 @@ func Rename(from, to string) error { defer os.Remove(from) return os.Rename(from, to) } + +// InWritableDir calls fn(path), while making sure that the directory +// containing `path` is writable for the duration of the call. +func InWritableDir(fn func(string) error, path string) error { + dir := filepath.Dir(path) + if info, err := os.Stat(dir); err == nil && info.IsDir() && info.Mode()&04 == 0 { + // A non-writeable directory (for this user; we assume that's the + // relevant part). Temporarily change the mode so we can delete the + // file or directory inside it. + err = os.Chmod(dir, 0755) + if err == nil { + defer func() { + err = os.Chmod(dir, info.Mode()) + if err != nil { + // We managed to change the permission bits like a + // millisecond ago, so it'd be bizarre if we couldn't + // change it back. + panic(err) + } + }() + } + } + + return fn(path) +}