Merge pull request #1885 from rumpl/fix/use-os-remove-instead-of-syscall-rmdir

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

Author: dgageot

pkg/tools/builtin/filesystem.go

@@ -3,7 +3,6 @@ package builtin
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io/fs"
 	"log/slog"
@@ -13,7 +12,6 @@ import (
 	"regexp"
 	"strings"
 	"sync"
-	"syscall"
 
 	"github.com/docker/cagent/pkg/fsx"
 	"github.com/docker/cagent/pkg/tools"
@@ -756,10 +754,7 @@ func (t *FilesystemTool) handleRemoveDirectory(_ context.Context, args RemoveDir
 	for _, path := range args.Paths {
 		resolvedPath := t.resolvePath(path)
 
-		if err := syscall.Rmdir(resolvedPath); err != nil {
-			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 {
 			return tools.ResultError(fmt.Sprintf("Error removing directory %s: %s", path, err)), nil
 		}
 		results = append(results, fmt.Sprintf("Directory removed successfully: %s", path))

pkg/tools/builtin/filesystem_test.go

@@ -738,7 +738,7 @@ func TestFilesystemTool_RemoveDirectory_IsFile(t *testing.T) {
 	})
 	require.NoError(t, err)
 	assert.True(t, result.IsError)
-	assert.Contains(t, result.Output, "is not a directory")
+	assert.Contains(t, result.Output, "not a directory")
 }
 
 func TestFilesystemTool_RemoveDirectory_MultipleStopsOnError(t *testing.T) {

pkg/tools/builtin/rmdir_unix.go

@@ -0,0 +1,12 @@
+//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris
+
+package builtin
+
+import "golang.org/x/sys/unix"
+
+// rmdir removes an empty directory. It returns an error if the path is not
+// a directory (e.g. ENOTDIR) without the TOCTOU race that a stat-then-remove
+// sequence would have, because unix.Rmdir is a single atomic syscall.
+func rmdir(path string) error {
+	return unix.Rmdir(path)
+}

pkg/tools/builtin/rmdir_windows.go

@@ -0,0 +1,14 @@
+package builtin
+
+import "golang.org/x/sys/windows"
+
+// rmdir removes an empty directory. It returns an error if the path is not
+// a directory without the TOCTOU race that a stat-then-remove sequence would
+// have, because windows.RemoveDirectory is a single atomic syscall.
+func rmdir(path string) error {
+	p, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return err
+	}
+	return windows.RemoveDirectory(p)
+}