From: Martin Tournoij Date: Sat, 6 Aug 2022 16:40:15 +0000 (+0200) Subject: kqueue: don't set up watchers on unreadable files (#479) X-Git-Tag: v1.7.2~69 X-Git-Url: https://go.fuhry.dev/?a=commitdiff_plain;h=8ab3b84129d248db26a9760f389b4c9686ba4031;p=fsnotify.git kqueue: don't set up watchers on unreadable files (#479) Watching an entire directory would fail if it contains a file we can't read due to permission errors. That's obviously not ideal, so just skip the file. Also add a test for trying to watch a directory where the permissions are denied, and test the error is what's expected; this prevents regressions like in #393. Fixes #439 --- diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e926f07..15bae7a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -42,7 +42,8 @@ jobs: usesh: true prepare: pkg install -y go run: | - go test -race ./... + pw user add -n action -m + su action -c 'go test -race ./...' testOpenBSD: runs-on: macos-12 @@ -54,18 +55,18 @@ jobs: uses: vmactions/openbsd-vm@v0.0.6 with: prepare: pkg_add go + # No -race as the VM doesn't include the comp set. + # + # TODO: should probably add this, but on my local machine the tests + # time out with -race as the waits aren't long enough (OpenBSD + # is kind of slow), so should probably look into that first. + # Go 1.19 is supposed to have a much faster race detector, so + # maybe waiting until we have that is enough. run: | # Default of 512 leads to "too many open files". ulimit -n 1024 - - # No -race as the VM doesn't include the comp set. - # - # TODO: should probably add this, but on my local machine the tests - # time out with -race as the waits aren't long enough (OpenBSD - # is kind of slow), so should probably look into that first. - # Go 1.19 is supposed to have a much faster race detector, so - # maybe waiting until we have that is enough. - go test ./... + useradd -mG wheel action + su action -c 'go test ./...' testNetBSD: runs-on: macos-12 @@ -77,9 +78,10 @@ jobs: uses: vmactions/netbsd-vm@v0.0.4 with: prepare: pkg_add go + # TODO: no -race for the same reason as OpenBSD (the timing; it does run). run: | - # TODO: no -race for the same reason as OpenBSD (the timing; it does run). - go117 test ./... + useradd -mG wheel action + su action -c 'go117 test ./...' testillumos: runs-on: macos-12 diff --git a/integration_test.go b/integration_test.go index 3367e80..da9bb20 100644 --- a/integration_test.go +++ b/integration_test.go @@ -4,6 +4,7 @@ package fsnotify import ( + "errors" "fmt" "path/filepath" "runtime" @@ -11,6 +12,8 @@ import ( "sync/atomic" "testing" "time" + + "github.com/fsnotify/fsnotify/internal" ) func TestWatch(t *testing.T) { @@ -79,6 +82,31 @@ func TestWatch(t *testing.T) { remove /sub remove /file `}, + + {"file in directory is not readable", func(t *testing.T, w *Watcher, tmp string) { + if runtime.GOOS == "windows" { + t.Skip("attributes don't work on Windows") + } + + touch(t, tmp, "file-unreadable") + chmod(t, 0, tmp, "file-unreadable") + touch(t, tmp, "file") + addWatch(t, w, tmp) + + cat(t, "hello", tmp, "file") + rm(t, tmp, "file") + rm(t, tmp, "file-unreadable") + }, ` + WRITE "/file" + REMOVE "/file" + REMOVE "/file-unreadable" + + # We never set up a watcher on the unreadable file, so we don't get + # the REMOVE. + kqueue: + WRITE "/file" + REMOVE "/file" + `}, } for _, tt := range tests { @@ -445,6 +473,38 @@ func TestClose(t *testing.T) { }) } +func TestAdd(t *testing.T) { + t.Run("permission denied", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("attributes don't work on Windows") + } + + t.Parallel() + + tmp := t.TempDir() + dir := filepath.Join(tmp, "dir-unreadable") + mkdir(t, dir) + touch(t, dir, "/file") + chmod(t, 0, dir) + + w := newWatcher(t) + defer func() { + w.Close() + chmod(t, 0o755, dir) // Make TempDir() cleanup work + }() + err := w.Add(dir) + if err == nil { + t.Fatal("error is nil") + } + if !errors.Is(err, internal.UnixEACCES) { + t.Errorf("not unix.EACCESS: %T %#[1]v", err) + } + if !errors.Is(err, internal.SyscallEACCES) { + t.Errorf("not syscall.EACCESS: %T %#[1]v", err) + } + }) +} + // TODO: should also check internal state is correct/cleaned up; e.g. no // left-over file descriptors or whatnot. func TestRemove(t *testing.T) { diff --git a/internal/unix.go b/internal/unix.go new file mode 100644 index 0000000..7c73a9d --- /dev/null +++ b/internal/unix.go @@ -0,0 +1,15 @@ +//go:build !windows +// +build !windows + +package internal + +import ( + "syscall" + + "golang.org/x/sys/unix" +) + +var ( + SyscallEACCES = syscall.EACCES + UnixEACCES = unix.EACCES +) diff --git a/internal/windows.go b/internal/windows.go new file mode 100644 index 0000000..6f20859 --- /dev/null +++ b/internal/windows.go @@ -0,0 +1,14 @@ +//go:build windows +// +build windows + +package internal + +import ( + "errors" +) + +// Just a dummy. +var ( + SyscallEACCES = errors.New("dummy") + UnixEACCES = errors.New("dummy") +) diff --git a/kqueue.go b/kqueue.go index c63e7b6..765e85f 100644 --- a/kqueue.go +++ b/kqueue.go @@ -199,13 +199,8 @@ func (w *Watcher) addWatch(name string, flags uint32) (string, error) { return "", err } - // Don't watch sockets. - if fi.Mode()&os.ModeSocket == os.ModeSocket { - return "", nil - } - - // Don't watch named pipes. - if fi.Mode()&os.ModeNamedPipe == os.ModeNamedPipe { + // Don't watch sockets or named pipes + if (fi.Mode()&os.ModeSocket == os.ModeSocket) || (fi.Mode()&os.ModeNamedPipe == os.ModeNamedPipe) { return "", nil } @@ -427,14 +422,24 @@ func (w *Watcher) watchDirectoryFiles(dirPath string) error { } for _, fileInfo := range files { - filePath := filepath.Join(dirPath, fileInfo.Name()) - filePath, err = w.internalWatch(filePath, fileInfo) + path := filepath.Join(dirPath, fileInfo.Name()) + + cleanPath, err := w.internalWatch(path, fileInfo) if err != nil { - return fmt.Errorf("%q: %w", filepath.Join(dirPath, fileInfo.Name()), err) + // No permission to read the file; that's not a problem: just skip. + // But do add it to w.fileExists to prevent it from being picked up + // as a "new" file later (it still shows up in the directory + // listing). + switch { + case errors.Is(err, unix.EACCES) || errors.Is(err, unix.EPERM): + cleanPath = filepath.Clean(path) + default: + return fmt.Errorf("%q: %w", filepath.Join(dirPath, fileInfo.Name()), err) + } } w.mu.Lock() - w.fileExists[filePath] = struct{}{} + w.fileExists[cleanPath] = struct{}{} w.mu.Unlock() }