diff --git a/cmd/ooniprobe/internal/autorun/autorun_darwin.go b/cmd/ooniprobe/internal/autorun/autorun_darwin.go index ee03cb4044..fe35e8a895 100644 --- a/cmd/ooniprobe/internal/autorun/autorun_darwin.go +++ b/cmd/ooniprobe/internal/autorun/autorun_darwin.go @@ -11,7 +11,7 @@ import ( "text/template" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/utils" + "github.com/ooni/probe-cli/v3/internal/fsx" "github.com/ooni/probe-cli/v3/internal/shellx" "golang.org/x/sys/execabs" "golang.org/x/sys/unix" @@ -90,7 +90,7 @@ func (managerDarwin) LogStream() error { func (managerDarwin) mustNotHavePlist() error { log.Infof("exec: test -f %s && already_registered()", plistPath) - if utils.FileExists(plistPath) { + if fsx.RegularFileExists(plistPath) { // This is not atomic. Do we need atomicity here? return errors.New("autorun: service already registered") } diff --git a/cmd/ooniprobe/internal/utils/paths.go b/cmd/ooniprobe/internal/utils/paths.go index ca5f88ecc8..4377def3ed 100644 --- a/cmd/ooniprobe/internal/utils/paths.go +++ b/cmd/ooniprobe/internal/utils/paths.go @@ -6,6 +6,7 @@ import ( "path/filepath" "github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/utils/homedir" + "github.com/ooni/probe-cli/v3/internal/fsx" ) // RequiredDirs returns the required ooni home directories @@ -44,13 +45,6 @@ func DBDir(home string, name string) string { return filepath.Join(home, "db", fmt.Sprintf("%s.sqlite3", name)) } -// FileExists returns true if the specified path exists and is a -// regular file. -func FileExists(path string) bool { - stat, err := os.Stat(path) - return err == nil && stat.Mode().IsRegular() -} - // GetOONIHome returns the path to the OONI Home func GetOONIHome() (string, error) { if ooniHome := os.Getenv("OONI_HOME"); ooniHome != "" { @@ -74,5 +68,5 @@ func DidLegacyInformedConsent() bool { } path := filepath.Join(filepath.Join(home, ".ooni"), "initialized") - return FileExists(path) + return fsx.RegularFileExists(path) } diff --git a/internal/cmd/miniooni/consent.go b/internal/cmd/miniooni/consent.go index 50f205a25b..3edd8710d5 100644 --- a/internal/cmd/miniooni/consent.go +++ b/internal/cmd/miniooni/consent.go @@ -8,6 +8,7 @@ import ( "os" "path" + "github.com/ooni/probe-cli/v3/internal/fsx" "github.com/ooni/probe-cli/v3/internal/runtimex" ) @@ -18,7 +19,7 @@ func acquireUserConsent(miniooniDir string, currentOptions *Options) { err := maybeWriteConsentFile(currentOptions.Yes, consentFile) runtimex.PanicOnError(err, "cannot write informed consent file") runtimex.Assert( - regularFileExists(consentFile), + fsx.RegularFileExists(consentFile), riskOfRunningOONI, ) } diff --git a/internal/cmd/miniooni/utils.go b/internal/cmd/miniooni/utils.go index cb51e13398..6deb74ee44 100644 --- a/internal/cmd/miniooni/utils.go +++ b/internal/cmd/miniooni/utils.go @@ -14,12 +14,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/runtimex" ) -// regularFileExists returns true if the given filepath exists and is a regular file -func regularFileExists(filepath string) bool { - stat, err := os.Stat(filepath) - return err == nil && stat.Mode().IsRegular() -} - // splitPair takes in input a string in the form KEY=VALUE and splits it. This // function returns an error if it cannot find the = character to split the string. func splitPair(s string) (string, string, error) { diff --git a/internal/cmd/oonireport/oonireport.go b/internal/cmd/oonireport/oonireport.go index 80dddb45ad..69cd5eedef 100644 --- a/internal/cmd/oonireport/oonireport.go +++ b/internal/cmd/oonireport/oonireport.go @@ -13,6 +13,7 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/fsx" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/probeservices" "github.com/ooni/probe-cli/v3/internal/runtimex" @@ -52,11 +53,6 @@ func fatalIfFalse(cond bool, msg string) { } } -func canOpen(filepath string) bool { - stat, err := os.Stat(filepath) - return err == nil && stat.Mode().IsRegular() -} - func readLines(path string) []string { // open measurement file file, err := os.Open(path) @@ -127,7 +123,7 @@ func submitAll(ctx context.Context, lines []string, subm *probeservices.Submitte func mainWithArgs(args []string) { fatalIfFalse(len(args) == 2, "Usage: ./oonireport upload ") fatalIfFalse(args[0] == "upload", "Unsupported operation") - fatalIfFalse(canOpen(args[1]), "Cannot open measurement file") + fatalIfFalse(fsx.RegularFileExists(args[1]), "Cannot open measurement file") path = args[1] lines := readLines(path) diff --git a/internal/cmd/oonireport/oonireport_test.go b/internal/cmd/oonireport/oonireport_test.go index e93705c092..0a2f630670 100644 --- a/internal/cmd/oonireport/oonireport_test.go +++ b/internal/cmd/oonireport/oonireport_test.go @@ -5,13 +5,6 @@ import ( "testing" ) -func TestCanOpen(t *testing.T) { - ok := canOpen("testdata/testmeasurement.json") - if !ok { - t.Fatal("unexpected error") - } -} - func TestReadLines(t *testing.T) { lines := readLines("testdata/testmeasurement.json") if lines == nil { diff --git a/internal/fsx/fsx.go b/internal/fsx/fsx.go index b2f9dcd1f8..2900d588b3 100644 --- a/internal/fsx/fsx.go +++ b/internal/fsx/fsx.go @@ -2,15 +2,15 @@ package fsx import ( + "errors" + "fmt" "io/fs" "os" - "syscall" ) // OpenFile is a wrapper for os.OpenFile that ensures that // we're opening a file rather than a directory. If you are -// opening a directory, this func returns an *os.PathError -// error with Err set to syscall.EISDIR. +// not opening a regular file, this func returns an error. // // As mentioned in CONTRIBUTING.md, this is the function // you SHOULD be using when opening files. @@ -18,6 +18,9 @@ func OpenFile(pathname string) (fs.File, error) { return openWithFS(filesystem{}, pathname) } +// ErrNotRegularFile indicates you're not opening a regular file. +var ErrNotRegularFile = errors.New("not a regular file") + // openWithFS is like Open but with explicit file system argument. func openWithFS(fs fs.FS, pathname string) (fs.File, error) { file, err := fs.Open(pathname) @@ -29,13 +32,9 @@ func openWithFS(fs fs.FS, pathname string) (fs.File, error) { file.Close() return nil, err } - if info.IsDir() { + if !isRegular(info) { file.Close() - return nil, &os.PathError{ - Op: "openFile", - Path: pathname, - Err: syscall.EISDIR, - } + return nil, fmt.Errorf("%w: %s", ErrNotRegularFile, pathname) } return file, nil } @@ -47,3 +46,17 @@ type filesystem struct{} func (filesystem) Open(pathname string) (fs.File, error) { return os.Open(pathname) } + +func isRegular(info fs.FileInfo) bool { + return info.Mode().IsRegular() +} + +// RegularFileExists returns whether the given filename +// exists and is a regular file. +func RegularFileExists(filename string) bool { + finfo, err := os.Stat(filename) + if err != nil { + return false + } + return isRegular(finfo) +} diff --git a/internal/fsx/fsx_test.go b/internal/fsx/fsx_test.go index 3941f1869d..b88b12a9d1 100644 --- a/internal/fsx/fsx_test.go +++ b/internal/fsx/fsx_test.go @@ -4,6 +4,8 @@ import ( "errors" "io/fs" "os" + "path/filepath" + "runtime" "sync/atomic" "syscall" "testing" @@ -69,7 +71,7 @@ func TestOpenNonexistentFile(t *testing.T) { func TestOpenDirectoryShouldFail(t *testing.T) { _, err := OpenFile(baseDir) - if !errors.Is(err, syscall.EISDIR) { + if !errors.Is(err, ErrNotRegularFile) { t.Fatalf("not the error we expected: %+v", err) } } @@ -81,3 +83,38 @@ func TestOpeningExistingFileShouldWork(t *testing.T) { } defer file.Close() } + +func TestRegularFileExists(t *testing.T) { + t.Run("for existing file", func(t *testing.T) { + path := filepath.Join("testdata", "testfile.txt") + exists := RegularFileExists(path) + if !exists { + t.Fatal("should exist") + } + }) + + t.Run("for existing directory", func(t *testing.T) { + exists := RegularFileExists("testdata") + if exists { + t.Fatal("should not exist") + } + }) + + t.Run("for nonexisting file", func(t *testing.T) { + path := filepath.Join("testdata", "nonexistent") + exists := RegularFileExists(path) + if exists { + t.Fatal("should not exist") + } + }) + + t.Run("for a special file", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skip test under windows") + } + exists := RegularFileExists("/dev/null") + if exists { + t.Fatal("should not exist") + } + }) +}