From 407b783aa5e4c6b1b74ec120b2a873bf19f96dc3 Mon Sep 17 00:00:00 2001 From: Matthew Penner Date: Sun, 9 Mar 2025 19:19:29 -0600 Subject: [PATCH] ufs: improve error handling Signed-off-by: Matthew Penner --- internal/ufs/error.go | 175 ++++++++++++++++++++---------- internal/ufs/fs_quota.go | 16 ++- internal/ufs/fs_unix.go | 187 ++++++++++++++++++++++----------- internal/ufs/mkdir_unix.go | 6 +- internal/ufs/removeall_unix.go | 124 ++++++++++++++++------ internal/ufs/walk_unix.go | 4 +- 6 files changed, 347 insertions(+), 165 deletions(-) diff --git a/internal/ufs/error.go b/internal/ufs/error.go index dc563e4..842d404 100644 --- a/internal/ufs/error.go +++ b/internal/ufs/error.go @@ -7,6 +7,7 @@ import ( "errors" iofs "io/fs" "os" + "syscall" "golang.org/x/sys/unix" ) @@ -48,9 +49,9 @@ type PathError = iofs.PathError // SyscallError records an error from a specific system call. type SyscallError = os.SyscallError -// NewSyscallError returns, as an error, a new SyscallError -// with the given system call name and error details. -// As a convenience, if err is nil, NewSyscallError returns nil. +// NewSyscallError returns, as an error, a new [*os.SyscallError] with the +// given system call name and error details. As a convenience, if err is nil, +// [NewSyscallError] returns nil. func NewSyscallError(syscall string, err error) error { return os.NewSyscallError(syscall, err) } @@ -61,62 +62,122 @@ func convertErrorType(err error) error { if err == nil { return nil } + var pErr *PathError - switch { - case errors.As(err, &pErr): - switch { - // File exists - case errors.Is(pErr.Err, unix.EEXIST): - return &PathError{ - Op: pErr.Op, - Path: pErr.Path, - Err: ErrExist, - } - // Is a directory - case errors.Is(pErr.Err, unix.EISDIR): - return &PathError{ - Op: pErr.Op, - Path: pErr.Path, - Err: ErrIsDirectory, - } - // Not a directory - case errors.Is(pErr.Err, unix.ENOTDIR): - return &PathError{ - Op: pErr.Op, - Path: pErr.Path, - Err: ErrNotDirectory, - } - // No such file or directory - case errors.Is(pErr.Err, unix.ENOENT): - return &PathError{ - Op: pErr.Op, - Path: pErr.Path, - Err: ErrNotExist, - } - // Operation not permitted - case errors.Is(pErr.Err, unix.EPERM): - return &PathError{ - Op: pErr.Op, - Path: pErr.Path, - Err: ErrPermission, - } - // Invalid cross-device link - case errors.Is(pErr.Err, unix.EXDEV): - return &PathError{ - Op: pErr.Op, - Path: pErr.Path, - Err: ErrBadPathResolution, - } - // Too many levels of symbolic links - case errors.Is(pErr.Err, unix.ELOOP): - return &PathError{ - Op: pErr.Op, - Path: pErr.Path, - Err: ErrBadPathResolution, - } - // TODO: EROFS - // TODO: ENOTEMPTY + if errors.As(err, &pErr) { + if errno, ok := pErr.Err.(syscall.Errno); ok { + return errnoToPathError(errno, pErr.Op, pErr.Path) + } + return pErr + } + + // If the error wasn't already a path error and is a errno, wrap it with + // details that we can use to know there is something wrong with our + // error wrapping somewhere. + var errno syscall.Errno + if errors.As(err, &errno) { + return &PathError{ + Op: "!(UNKNOWN)", + Path: "!(UNKNOWN)", + Err: err, } } + return err } + +// ensurePathError ensures that err is a PathError. The op and path arguments +// are only used of the error isn't already a PathError. +func ensurePathError(err error, op, path string) error { + if err == nil { + return nil + } + + // Check if the error is already a PathError. + var pErr *PathError + if errors.As(err, &pErr) { + // If underlying error is a errno, convert it. + // + // DO NOT USE `errors.As` or whatever here, the error will either be + // an errno, or it will be wrapped already. + if errno, ok := pErr.Err.(syscall.Errno); ok { + return errnoToPathError(errno, pErr.Op, pErr.Path) + } + // Return the PathError as-is without modification. + return pErr + } + + // If the error is directly an errno, convert it to a PathError. + var errno syscall.Errno + if errors.As(err, &errno) { + return errnoToPathError(errno, op, path) + } + + // Otherwise just wrap it as a PathError without any additional changes. + return &PathError{ + Op: op, + Path: path, + Err: err, + } +} + +// errnoToPathError converts an errno into a proper path error. +func errnoToPathError(err syscall.Errno, op, path string) error { + switch err { + // File exists + case unix.EEXIST: + return &PathError{ + Op: op, + Path: path, + Err: ErrExist, + } + // Is a directory + case unix.EISDIR: + return &PathError{ + Op: op, + Path: path, + Err: ErrIsDirectory, + } + // Not a directory + case unix.ENOTDIR: + return &PathError{ + Op: op, + Path: path, + Err: ErrNotDirectory, + } + // No such file or directory + case unix.ENOENT: + return &PathError{ + Op: op, + Path: path, + Err: ErrNotExist, + } + // Operation not permitted + case unix.EPERM: + return &PathError{ + Op: op, + Path: path, + Err: ErrPermission, + } + // Invalid cross-device link + case unix.EXDEV: + return &PathError{ + Op: op, + Path: path, + Err: ErrBadPathResolution, + } + // Too many levels of symbolic links + case unix.ELOOP: + return &PathError{ + Op: op, + Path: path, + Err: ErrBadPathResolution, + } + default: + return &PathError{ + Op: op, + Path: path, + Err: err, + } + } +} diff --git a/internal/ufs/fs_quota.go b/internal/ufs/fs_quota.go index 600a9fa..cc89cbd 100644 --- a/internal/ufs/fs_quota.go +++ b/internal/ufs/fs_quota.go @@ -7,8 +7,12 @@ import ( "sync/atomic" ) -// Quota . -// TODO: document +// Quota is a wrapper around [*UnixFS] that provides the ability to limit the +// disk usage of the filesystem. +// +// NOTE: this is not a full complete quota filesystem, it provides utilities for +// tracking and checking the usage of the filesystem. The only operation that is +// automatically accounted against the quota are file deletions. type Quota struct { // fs is the underlying filesystem that runs the actual I/O operations. *UnixFS @@ -28,8 +32,7 @@ type Quota struct { usage atomic.Int64 } -// NewQuota . -// TODO: document +// NewQuota creates a new Quota filesystem using an existing UnixFS and a limit. func NewQuota(fs *UnixFS, limit int64) *Quota { qfs := Quota{UnixFS: fs} qfs.limit.Store(limit) @@ -105,6 +108,9 @@ func (fs *Quota) CanFit(size int64) bool { return false } +// Remove removes the named file or (empty) directory. +// +// If there is an error, it will be of type [*PathError]. func (fs *Quota) Remove(name string) error { // For information on why this interface is used here, check its // documentation. @@ -129,7 +135,7 @@ func (fs *Quota) Remove(name string) error { // it encounters. If the path does not exist, RemoveAll // returns nil (no error). // -// If there is an error, it will be of type *PathError. +// If there is an error, it will be of type [*PathError]. func (fs *Quota) RemoveAll(name string) error { name, err := fs.unsafePath(name) if err != nil { diff --git a/internal/ufs/fs_unix.go b/internal/ufs/fs_unix.go index 87b83c7..dff36c9 100644 --- a/internal/ufs/fs_unix.go +++ b/internal/ufs/fs_unix.go @@ -81,7 +81,16 @@ func (fs *UnixFS) Chmod(name string, mode FileMode) error { if err != nil { return err } - return convertErrorType(unix.Fchmodat(dirfd, name, uint32(mode), 0)) + return fs.fchmodat("chmod", dirfd, name, mode) +} + +// Chmodat is like Chmod but it takes a dirfd and name instead of a full path. +func (fs *UnixFS) Chmodat(dirfd int, name string, mode FileMode) error { + return fs.fchmodat("chmodat", dirfd, name, mode) +} + +func (fs *UnixFS) fchmodat(op string, dirfd int, name string, mode FileMode) error { + return ensurePathError(unix.Fchmodat(dirfd, name, uint32(mode), 0), op, name) } // Chown changes the numeric uid and gid of the named file. @@ -93,7 +102,7 @@ func (fs *UnixFS) Chmod(name string, mode FileMode) error { // On Windows or Plan 9, Chown always returns the syscall.EWINDOWS or // EPLAN9 error, wrapped in *PathError. func (fs *UnixFS) Chown(name string, uid, gid int) error { - return fs.fchown(name, uid, gid, 0) + return ensurePathError(fs.fchown(name, uid, gid, 0), "chown", name) } // Lchown changes the numeric uid and gid of the named file. @@ -106,7 +115,7 @@ func (fs *UnixFS) Chown(name string, uid, gid int) error { func (fs *UnixFS) Lchown(name string, uid, gid int) error { // With AT_SYMLINK_NOFOLLOW, Fchownat acts like Lchown but allows us to // pass a dirfd. - return fs.fchown(name, uid, gid, AT_SYMLINK_NOFOLLOW) + return ensurePathError(fs.fchown(name, uid, gid, AT_SYMLINK_NOFOLLOW), "lchown", name) } // fchown is a re-usable Fchownat syscall used by Chown and Lchown. @@ -116,19 +125,19 @@ func (fs *UnixFS) fchown(name string, uid, gid, flags int) error { if err != nil { return err } - return convertErrorType(unix.Fchownat(dirfd, name, uid, gid, flags)) + return unix.Fchownat(dirfd, name, uid, gid, flags) } // Chownat is like Chown but allows passing an existing directory file // descriptor rather than needing to resolve one. func (fs *UnixFS) Chownat(dirfd int, name string, uid, gid int) error { - return convertErrorType(unix.Fchownat(dirfd, name, uid, gid, 0)) + return ensurePathError(unix.Fchownat(dirfd, name, uid, gid, 0), "chownat", name) } // Lchownat is like Lchown but allows passing an existing directory file // descriptor rather than needing to resolve one. func (fs *UnixFS) Lchownat(dirfd int, name string, uid, gid int) error { - return convertErrorType(unix.Fchownat(dirfd, name, uid, gid, AT_SYMLINK_NOFOLLOW)) + return ensurePathError(unix.Fchownat(dirfd, name, uid, gid, AT_SYMLINK_NOFOLLOW), "lchownat", name) } // Chtimes changes the access and modification times of the named @@ -160,11 +169,9 @@ func (fs *UnixFS) Chtimesat(dirfd int, name string, atime, mtime time.Time) erro } set(0, atime) set(1, mtime) + // This does support `AT_SYMLINK_NOFOLLOW` as well if needed. - if err := unix.UtimesNanoAt(dirfd, name, utimes[0:], 0); err != nil { - return convertErrorType(&PathError{Op: "chtimes", Path: name, Err: err}) - } - return nil + return ensurePathError(unix.UtimesNanoAt(dirfd, name, utimes[0:], 0), "chtimes", name) } // Create creates or truncates the named file. If the file already exists, @@ -188,11 +195,15 @@ func (fs *UnixFS) Mkdir(name string, mode FileMode) error { if err != nil { return err } - return fs.Mkdirat(dirfd, name, mode) + return fs.mkdirat("mkdir", dirfd, name, mode) } func (fs *UnixFS) Mkdirat(dirfd int, name string, mode FileMode) error { - return convertErrorType(unix.Mkdirat(dirfd, name, uint32(mode))) + return fs.mkdirat("mkdirat", dirfd, name, mode) +} + +func (fs *UnixFS) mkdirat(op string, dirfd int, name string, mode FileMode) error { + return ensurePathError(unix.Mkdirat(dirfd, name, uint32(mode)), op, name) } // MkdirAll creates a directory named path, along with any necessary @@ -313,7 +324,7 @@ func (fs *UnixFS) RemoveStat(name string) (FileInfo, error) { err = fs.unlinkat(dirfd, name, 0) } if err != nil { - return s, convertErrorType(&PathError{Op: "remove", Path: name, Err: err}) + return s, ensurePathError(err, "rename", name) } return s, nil } @@ -362,7 +373,7 @@ func (fs *UnixFS) Remove(name string) error { if err1 != unix.ENOTDIR { err = err1 } - return convertErrorType(&PathError{Op: "remove", Path: name, Err: err}) + return ensurePathError(err, "remove", name) } // RemoveAll removes path and any children it contains. @@ -377,6 +388,7 @@ func (fs *UnixFS) RemoveAll(name string) error { if err != nil { return err } + // While removeAll internally checks this, I want to make sure we check it // and return the proper error so our tests can ensure that this will never // be a possibility. @@ -387,9 +399,29 @@ func (fs *UnixFS) RemoveAll(name string) error { Err: ErrBadPathResolution, } } + return fs.removeAll(name) } +// RemoveContents recursively removes the contents of name. +// +// It removes everything it can but returns the first error +// it encounters. If the path does not exist, RemoveContents +// returns nil (no error). +// +// If there is an error, it will be of type [*PathError]. +func (fs *UnixFS) RemoveContents(name string) error { + name, err := fs.unsafePath(name) + if err != nil { + return err + } + + // Unlike RemoveAll, we don't remove `name` itself, only it's contents. + // So there is no need to check for a name of `.` here. + + return fs.removeContents(name) +} + func (fs *UnixFS) unlinkat(dirfd int, name string, flags int) error { return ignoringEINTR(func() error { return unix.Unlinkat(dirfd, name, flags) @@ -418,11 +450,11 @@ func (fs *UnixFS) Rename(oldpath, newpath string) error { // While unix.Renameat ends up throwing a "device or resource busy" error, // that doesn't mean we are protecting the system properly. if oldname == "." { - return convertErrorType(&PathError{ + return &PathError{ Op: "rename", Path: oldname, Err: ErrBadPathResolution, - }) + } } // Stat the old target to return proper errors. if _, err := fs.Lstatat(olddirfd, oldname); err != nil { @@ -433,11 +465,11 @@ func (fs *UnixFS) Rename(oldpath, newpath string) error { if err != nil { closeFd2() if !errors.Is(err, ErrNotExist) { - return convertErrorType(err) + return err } var pathErr *PathError if !errors.As(err, &pathErr) { - return convertErrorType(err) + return err } if err := fs.MkdirAll(pathErr.Path, 0o755); err != nil { return err @@ -455,25 +487,28 @@ func (fs *UnixFS) Rename(oldpath, newpath string) error { // While unix.Renameat ends up throwing a "device or resource busy" error, // that doesn't mean we are protecting the system properly. if newname == "." { - return convertErrorType(&PathError{ + return &PathError{ Op: "rename", Path: newname, Err: ErrBadPathResolution, - }) + } } // Stat the new target to return proper errors. _, err = fs.Lstatat(newdirfd, newname) switch { case err == nil: - return convertErrorType(&PathError{ + return &PathError{ Op: "rename", Path: newname, Err: ErrExist, - }) + } case !errors.Is(err, ErrNotExist): return err } - return unix.Renameat(olddirfd, oldname, newdirfd, newname) + if err := unix.Renameat(olddirfd, oldname, newdirfd, newname); err != nil { + return &LinkError{Op: "rename", Old: oldpath, New: newpath, Err: err} + } + return nil } // Stat returns a FileInfo describing the named file. @@ -527,7 +562,7 @@ func (fs *UnixFS) _fstatat(op string, dirfd int, name string, flags int) (FileIn if err := ignoringEINTR(func() error { return unix.Fstatat(dirfd, name, &s.sys, flags) }); err != nil { - return nil, &PathError{Op: op, Path: name, Err: err} + return nil, ensurePathError(err, op, name) } fillFileStatFromSys(&s, name) return &s, nil @@ -563,23 +598,42 @@ func (fs *UnixFS) Touch(path string, flag int, mode FileMode) (File, error) { if flag&O_CREATE == 0 { flag |= O_CREATE } - dirfd, name, closeFd, err := fs.safePath(path) + dirfd, name, closeFd, err, _ := fs.TouchPath(path) defer closeFd() - if err == nil { - return fs.OpenFileat(dirfd, name, flag, mode) - } - if !errors.Is(err, ErrNotExist) { + if err != nil { return nil, err } + return fs.OpenFileat(dirfd, name, flag, mode) +} + +// TouchPath is like SafePath except that it will create any missing directories +// in the path. Unlike SafePath, TouchPath returns an additional boolean which +// indicates whether the parent directories already existed, this is intended to +// be used as a way to know if the final destination could already exist. +func (fs *UnixFS) TouchPath(path string) (int, string, func(), error, bool) { + dirfd, name, closeFd, err := fs.safePath(path) + switch { + case err == nil: + return dirfd, name, closeFd, nil, true + case !errors.Is(err, ErrNotExist): + return dirfd, name, closeFd, err, false + } + var pathErr *PathError if !errors.As(err, &pathErr) { - return nil, err + return dirfd, name, closeFd, err, false } if err := fs.MkdirAll(pathErr.Path, 0o755); err != nil { - return nil, err + return dirfd, name, closeFd, err, false } - // Try to open the file one more time after creating its parent directories. - return fs.OpenFile(path, flag, mode) + + // Close the previous file descriptor since we are going to be opening + // a new one. + closeFd() + + // Run safe path again now that the parent directories have been created. + dirfd, name, closeFd, err = fs.safePath(path) + return dirfd, name, closeFd, err, false } // WalkDir walks the file tree rooted at root, calling fn for each file or @@ -621,7 +675,7 @@ func (fs *UnixFS) openat(dirfd int, name string, flag int, mode FileMode) (int, if err == unix.EINTR { continue } - return -1, convertErrorType(err) + return 0, err } // If we are using openat2, we don't need the additional security checks. @@ -646,24 +700,29 @@ func (fs *UnixFS) openat(dirfd int, name string, flag int, mode FileMode) (int, if !errors.As(err, &pErr) { return fd, fmt.Errorf("failed to evaluate symlink: %w", convertErrorType(err)) } + + // Update the final path to whatever directory or path didn't exist while + // recursing any symlinks. finalPath = pErr.Path + // Ensure the error is wrapped correctly. + err = convertErrorType(err) } // Check if the path is within our root. if !fs.unsafeIsPathInsideOfBase(finalPath) { - return fd, convertErrorType(&PathError{ - Op: "openat", + op := "openat" + if fs.useOpenat2 { + op = "openat2" + } + return fd, &PathError{ + Op: op, Path: name, Err: ErrBadPathResolution, - }) + } } - // This handles any hanging errors. - if err != nil { - return fd, convertErrorType(err) - } - - return fd, nil + // Return the file descriptor and any potential error. + return fd, err } // _openat is a wrapper around unix.Openat. This method should never be directly @@ -681,11 +740,11 @@ func (fs *UnixFS) _openat(dirfd int, name string, flag int, mode uint32) (int, e case err == nil: return fd, nil case err == unix.EINTR: - return -1, err + return fd, err case err == unix.EAGAIN: - return -1, err + return fd, err default: - return -1, &PathError{Op: "openat", Path: name, Err: err} + return fd, ensurePathError(err, "openat", name) } } @@ -720,11 +779,11 @@ func (fs *UnixFS) _openat2(dirfd int, name string, flag, mode uint64) (int, erro case err == nil: return fd, nil case err == unix.EINTR: - return -1, err + return fd, err case err == unix.EAGAIN: - return -1, err + return fd, err default: - return -1, &PathError{Op: "openat2", Path: name, Err: err} + return fd, ensurePathError(err, "openat2", name) } } @@ -746,7 +805,7 @@ func (fs *UnixFS) safePath(path string) (dirfd int, file string, closeFd func(), // Open the base path. We use this as the sandbox root for any further // operations. var fsDirfd int - fsDirfd, err = unix.Openat(AT_EMPTY_PATH, fs.basePath, O_DIRECTORY|O_RDONLY, 0) + fsDirfd, err = fs._openat(AT_EMPTY_PATH, fs.basePath, O_DIRECTORY|O_RDONLY, 0) if err != nil { return } @@ -772,7 +831,7 @@ func (fs *UnixFS) safePath(path string) (dirfd int, file string, closeFd func(), // An error occurred while opening the directory, but we already opened // the filesystem root, so we still need to ensure it gets closed. closeFd = func() { _ = unix.Close(fsDirfd) } - } else { + } else { // Set closeFd to close the newly opened directory file descriptor. closeFd = func() { _ = unix.Close(dirfd) @@ -780,22 +839,24 @@ func (fs *UnixFS) safePath(path string) (dirfd int, file string, closeFd func(), } } - // Return dirfd, name, the closeFd func, and err return } -// unsafePath prefixes the given path and prefixes it with the filesystem's -// base path, cleaning the result. The path returned by this function may not -// be inside the filesystem's base path, additional checks are required to -// safely use paths returned by this function. +// unsafePath strips and joins the given path with the filesystem's base path, +// cleaning the result. The cleaned path is then checked if it starts with the +// filesystem's base path to obvious any obvious path traversal escapes. The +// fully resolved path (if symlinks are followed) may not be within the +// filesystem's base path, additional checks are required to safely use paths +// returned by this function. func (fs *UnixFS) unsafePath(path string) (string, error) { - // Calling filepath.Clean on the joined directory will resolve it to the - // absolute path, removing any ../ type of resolution arguments, and leaving - // us with a direct path link. + // Calling filepath.Clean on the path will resolve it to it's absolute path, + // removing any path traversal arguments (such as ..), leaving us with an + // absolute path we can then use. // - // This will also trim the existing root path off the beginning of the path - // passed to the function since that can get a bit messy. + // This will also trim the filesystem's base path from the given path and + // join the base path back on to ensure the path starts with the base path + // without appending it twice. r := filepath.Clean(filepath.Join(fs.basePath, strings.TrimPrefix(path, fs.basePath))) if fs.unsafeIsPathInsideOfBase(r) { @@ -822,6 +883,10 @@ func (fs *UnixFS) unsafePath(path string) (string, error) { // unsafeIsPathInsideOfBase checks if the given path is inside the filesystem's // base path. +// +// NOTE: this method doesn't clean the given path or attempt to join the +// filesystem's base path. This is purely a basic prefix check against the +// given path. func (fs *UnixFS) unsafeIsPathInsideOfBase(path string) bool { return strings.HasPrefix( strings.TrimSuffix(path, "/")+"/", diff --git a/internal/ufs/mkdir_unix.go b/internal/ufs/mkdir_unix.go index 88d3938..eb2942b 100644 --- a/internal/ufs/mkdir_unix.go +++ b/internal/ufs/mkdir_unix.go @@ -10,10 +10,6 @@ package ufs -import ( - "golang.org/x/sys/unix" -) - // mkdirAll is a recursive Mkdir implementation that properly handles symlinks. func (fs *UnixFS) mkdirAll(name string, mode FileMode) error { // Fast path: if we can tell whether path is a directory or file, stop with success or error. @@ -30,7 +26,7 @@ func (fs *UnixFS) mkdirAll(name string, mode FileMode) error { if dir.IsDir() { return nil } - return convertErrorType(&PathError{Op: "mkdir", Path: name, Err: unix.ENOTDIR}) + return &PathError{Op: "mkdir", Path: name, Err: ErrNotDirectory} } // Slow path: make sure parent exists and then call Mkdir for path. diff --git a/internal/ufs/removeall_unix.go b/internal/ufs/removeall_unix.go index 38a6e07..d756021 100644 --- a/internal/ufs/removeall_unix.go +++ b/internal/ufs/removeall_unix.go @@ -52,60 +52,69 @@ func removeAll(fs unixFS, path string) error { parentDir, base := splitPath(path) parent, err := fs.Open(parentDir) - if errors.Is(err, ErrNotExist) { + if err != nil { + if !errors.Is(err, ErrNotExist) { + return err + } // If parent does not exist, base cannot exist. Fail silently return nil } - if err != nil { - return err - } defer parent.Close() if err := removeAllFrom(fs, parent, base); err != nil { if pathErr, ok := err.(*PathError); ok { pathErr.Path = parentDir + string(os.PathSeparator) + pathErr.Path - err = pathErr + err = convertErrorType(pathErr) + } else { + err = ensurePathError(err, "removeallfrom", base) } - return convertErrorType(err) + return err } return nil } -func removeAllFrom(fs unixFS, parent File, base string) error { - parentFd := int(parent.Fd()) - // Simple case: if Unlink (aka remove) works, we're done. - err := fs.unlinkat(parentFd, base, 0) - if err == nil || errors.Is(err, ErrNotExist) { +func (fs *UnixFS) removeContents(path string) error { + return removeContents(fs, path) +} + +func removeContents(fs unixFS, path string) error { + if path == "" { + // fail silently to retain compatibility with previous behavior + // of RemoveAll. See issue https://go.dev/issue/28830. return nil } - // EISDIR means that we have a directory, and we need to - // remove its contents. - // EPERM or EACCES means that we don't have write permission on - // the parent directory, but this entry might still be a directory - // whose contents need to be removed. - // Otherwise, just return the error. - if err != unix.EISDIR && err != unix.EPERM && err != unix.EACCES { - return &PathError{Op: "unlinkat", Path: base, Err: err} - } + // RemoveAll recurses by deleting the path base from + // its parent directory + parentDir, base := splitPath(path) - // Is this a directory we need to recurse into? - var statInfo unix.Stat_t - statErr := ignoringEINTR(func() error { - return unix.Fstatat(parentFd, base, &statInfo, AT_SYMLINK_NOFOLLOW) - }) - if statErr != nil { - if errors.Is(statErr, ErrNotExist) { - return nil + parent, err := fs.Open(parentDir) + if err != nil { + if !errors.Is(err, ErrNotExist) { + return err } - return &PathError{Op: "fstatat", Path: base, Err: statErr} - } - if statInfo.Mode&unix.S_IFMT != unix.S_IFDIR { - // Not a directory; return the error from the unix.Unlinkat. - return &PathError{Op: "unlinkat", Path: base, Err: err} + // If parent does not exist, base cannot exist. Fail silently + return nil } + defer parent.Close() + + if err := removeContentsFrom(fs, parent, base); err != nil { + if pathErr, ok := err.(*PathError); ok { + pathErr.Path = parentDir + string(os.PathSeparator) + pathErr.Path + err = convertErrorType(pathErr) + } else { + err = ensurePathError(err, "removecontentsfrom", base) + } + return err + } + return nil +} + +// removeContentsFrom recursively removes all descendants of parent without +// removing parent itself. Parent must be a directory. +func removeContentsFrom(fs unixFS, parent File, base string) error { + parentFd := int(parent.Fd()) - // Remove the directory's entries. var recurseErr error for { const reqSize = 1024 @@ -168,6 +177,50 @@ func removeAllFrom(fs unixFS, parent File, base string) error { } } + return nil +} + +func removeAllFrom(fs unixFS, parent File, base string) error { + parentFd := int(parent.Fd()) + + // Simple case: if Unlink (aka remove) works, we're done. + err := fs.unlinkat(parentFd, base, 0) + if err == nil || errors.Is(err, ErrNotExist) { + return nil + } + + // EISDIR means that we have a directory, and we need to + // remove its contents. + // EPERM or EACCES means that we don't have write permission on + // the parent directory, but this entry might still be a directory + // whose contents need to be removed. + // Otherwise, just return the error. + if err != unix.EISDIR && err != unix.EPERM && err != unix.EACCES { + return &PathError{Op: "unlinkat", Path: base, Err: err} + } + + // Is this a directory we need to recurse into? + var statInfo unix.Stat_t + statErr := ignoringEINTR(func() error { + return unix.Fstatat(parentFd, base, &statInfo, AT_SYMLINK_NOFOLLOW) + }) + if statErr != nil { + if errors.Is(statErr, ErrNotExist) { + return nil + } + return &PathError{Op: "fstatat", Path: base, Err: statErr} + } + if statInfo.Mode&unix.S_IFMT != unix.S_IFDIR { + // Not a directory; return the error from the unix.Unlinkat. + return &PathError{Op: "unlinkat", Path: base, Err: err} + } + + // Remove all contents will remove the contents of the directory. + // + // It was split out of this function to allow the deletion of the + // contents of a directory, without deleting the directory itself. + recurseErr := removeContentsFrom(fs, parent, base) + // Remove the directory itself. unlinkErr := fs.unlinkat(parentFd, base, AT_REMOVEDIR) if unlinkErr == nil || errors.Is(unlinkErr, ErrNotExist) { @@ -177,7 +230,8 @@ func removeAllFrom(fs unixFS, parent File, base string) error { if recurseErr != nil { return recurseErr } - return &PathError{Op: "unlinkat", Path: base, Err: unlinkErr} + + return ensurePathError(err, "unlinkat", base) } // openFdAt opens path relative to the directory in fd. diff --git a/internal/ufs/walk_unix.go b/internal/ufs/walk_unix.go index 7b24130..065afc2 100644 --- a/internal/ufs/walk_unix.go +++ b/internal/ufs/walk_unix.go @@ -199,7 +199,7 @@ func (fs *UnixFS) modeTypeFromDirent(de *unix.Dirent, fd int, name string) (File func (fs *UnixFS) modeType(dirfd int, name string) (FileMode, error) { fi, err := fs.Lstatat(dirfd, name) if err != nil { - return 0, fmt.Errorf("ufs: error finding mode type for %s during readDir: %w", name, convertErrorType(err)) + return 0, fmt.Errorf("ufs: error finding mode type for %s during readDir: %w", name, err) } return fi.Mode() & ModeType, nil } @@ -227,7 +227,7 @@ func (fs *UnixFS) readDir(fd int, name, relative string, b []byte) ([]DirEntry, if err == unix.EINTR { continue } - return nil, fmt.Errorf("ufs: error with getdents during readDir: %w", convertErrorType(err)) + return nil, ensurePathError(err, "getdents", name) } if n <= 0 { // end of directory: normal exit