Skip to content

Replace syscall.Rmdir with golang.org/x/sys for cross-platform directory removal#1885

Open
rumpl wants to merge 1 commit intodocker:mainfrom
rumpl:fix/use-os-remove-instead-of-syscall-rmdir
Open

Replace syscall.Rmdir with golang.org/x/sys for cross-platform directory removal#1885
rumpl wants to merge 1 commit intodocker:mainfrom
rumpl:fix/use-os-remove-instead-of-syscall-rmdir

Conversation

@rumpl
Copy link
Member

@rumpl rumpl commented Mar 2, 2026

Summary

Replace syscall.Rmdir with platform-specific implementations using golang.org/x/sys, the supported successor to the frozen syscall package.

Changes

  • rmdir_unix.go — uses unix.Rmdir on aix/darwin/dragonfly/freebsd/linux/netbsd/openbsd/solaris
  • rmdir_windows.go — uses windows.RemoveDirectory
  • filesystem.go — calls the new rmdir() helper, removes syscall and errors imports
  • filesystem_test.go — relaxes assertion to match OS-level error string ("not a directory" vs "is not a directory")

Both unix.Rmdir and windows.RemoveDirectory are atomic, directory-only syscalls — they will not accidentally delete regular files and avoid the TOCTOU race that an os.Lstat + os.Remove sequence would introduce.

golang.org/x/sys is already a dependency of this project.

Fixes #1884

…ory removal

Use platform-specific files with golang.org/x/sys/unix and
golang.org/x/sys/windows instead of the frozen syscall package.
Both unix.Rmdir and windows.RemoveDirectory are atomic, directory-only
syscalls, avoiding the TOCTOU race that an os.Lstat + os.Remove
sequence would introduce.

Fixes docker#1884

Assisted-By: cagent
@rumpl rumpl requested a review from a team as a code owner March 2, 2026 07:34
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR successfully modernizes the directory removal implementation by replacing the frozen syscall package with platform-specific implementations from golang.org/x/sys. The changes are well-structured with proper build tags and maintain atomicity guarantees.

However, there is one notable behavioral change in error handling that should be considered.

if errors.Is(err, syscall.ENOTDIR) {
return tools.ResultError(fmt.Sprintf("Error: %s is not a directory", path)), nil
}
if err := rmdir(resolvedPath); err != nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loss of type-safe ENOTDIR error handling

The old implementation explicitly checked for syscall.ENOTDIR using errors.Is() to distinguish "not a directory" errors from other failure types (permission denied, path not found, etc.). The new implementation removes this check and now relies on error message strings.

While the underlying system calls (unix.Rmdir and windows.RemoveDirectory) do return appropriate error types/messages, callers that previously depended on type-safe error detection would now need to parse error strings. This is more fragile and platform-dependent.

Impact: Moderate - the functionality still works and the test passes, but error handling becomes less robust and harder to reason about programmatically.

Consider: If maintaining the explicit ENOTDIR check is important for API consumers, you could wrap the platform-specific errors and check for the appropriate error types (unix.ENOTDIR on Unix, windows.ERROR_DIRECTORY on Windows) before returning the generic error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] syscall.Rmdir is not cross-platform and may lead to unexpected behavior on Windows

1 participant