Improve uninstall on Windows to remove directory even when uninstall …

…is being executed from same directory (#3120)

* Revert the revert of the uninstall.

* Add retry for ERROR_SHARING_VIOLATION.

* Add comment.

changelog/fragments/1688657261-Don't-trigger-IOC-alert-on-Windows-uninstall.yaml

@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: bug-fix
+
+# Change summary; a 80ish characters long description of the change.
+summary: Don't trigger IOC alert on Windows uninstall
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+description:
+
+# Affected component; a word indicating the component this changeset affects.
+component: uninstall
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/3014
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/elastic-agent/issues/2970

internal/pkg/agent/install/uninstall.go

@@ -8,10 +8,10 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"runtime"
 	"strings"
+	"time"
 
 	"github.com/kardianos/service"
 
@@ -78,13 +78,8 @@ func Uninstall(cfgFile, topPath, uninstallToken string) error {
 	}
 
 	// remove existing directory
-	err = os.RemoveAll(topPath)
+	err = RemovePath(topPath)
 	if err != nil {
-		if runtime.GOOS == "windows" { //nolint:goconst // it is more readable this way
-			// possible to fail on Windows, because elastic-agent.exe is running from
-			// this directory.
-			return nil
-		}
 		return errors.New(
 			err,
 			fmt.Sprintf("failed to remove installation directory (%s)", paths.Top()),
@@ -95,15 +90,37 @@ func Uninstall(cfgFile, topPath, uninstallToken string) error {
 }
 
 // RemovePath helps with removal path where there is a probability
-// of running into self which might prevent removal.
-// Removal will be initiated 2 seconds after a call.
+// of running into an executable running that might prevent removal
+// on Windows.
+//
+// On Windows it is possible that a removal can spuriously error due
+// to an ERROR_SHARING_VIOLATION. RemovePath will retry up to 2
+// seconds if it keeps getting that error.
 func RemovePath(path string) error {
-	cleanupErr := os.RemoveAll(path)
-	if cleanupErr != nil && isBlockingOnSelf(cleanupErr) {
-		delayedRemoval(path)
+	const arbitraryTimeout = 2 * time.Second
+	var start time.Time
+	nextSleep := 1 * time.Millisecond
+	for {
+		err := os.RemoveAll(path)
+		if err == nil {
+			return nil
+		}
+		if isBlockingOnExe(err) {
+			// try to remove the blocking exe
+			err = removeBlockingExe(err)
+		}
+		if err == nil {
+			return nil
+		}
+		if !isRetryableError(err) {
+			return err
+		}
+		if start.IsZero() {
+			start = time.Now()
+		} else if d := time.Since(start) + nextSleep; d >= arbitraryTimeout {
+			return err
+		}
 	}
-
-	return cleanupErr
 }
 
 func RemoveBut(path string, bestEffort bool, exceptions ...string) error {
@@ -146,27 +163,6 @@ func containsString(str string, a []string, caseSensitive bool) bool {
 	return false
 }
 
-func isBlockingOnSelf(err error) bool {
-	// cannot remove self, this is expected on windows
-	// fails with  remove {path}}\elastic-agent.exe: Access is denied
-	return runtime.GOOS == "windows" &&
-		err != nil &&
-		strings.Contains(err.Error(), "elastic-agent.exe") &&
-		strings.Contains(err.Error(), "Access is denied")
-}
-
-func delayedRemoval(path string) {
-	// The installation path will still exists because we are executing from that
-	// directory. So cmd.exe is spawned that sleeps for 2 seconds (using ping, recommend way from
-	// from Windows) then rmdir is performed.
-	//nolint:gosec // it's not tainted
-	rmdir := exec.Command(
-		filepath.Join(os.Getenv("windir"), "system32", "cmd.exe"),
-		"/C", "ping", "-n", "2", "127.0.0.1", "&&", "rmdir", "/s", "/q", path)
-	_ = rmdir.Start()
-
-}
-
 func uninstallComponents(ctx context.Context, cfgFile string, uninstallToken string) error {
 	log, err := logger.NewWithLogpLevel("", logp.ErrorLevel, false)
 	if err != nil {

internal/pkg/agent/install/uninstall_unix.go

@@ -0,0 +1,19 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+//go:build !windows
+
+package install
+
+func isBlockingOnExe(_ error) bool {
+	return false
+}
+
+func removeBlockingExe(_ error) error {
+	return nil
+}
+
+func isRetryableError(_ error) bool {
+	return false
+}

internal/pkg/agent/install/uninstall_windows.go

@@ -0,0 +1,162 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+//go:build windows
+
+package install
+
+import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"syscall"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+func isBlockingOnExe(err error) bool {
+	if err == nil {
+		return false
+	}
+	path, errno := getPathFromError(err)
+	if path == "" {
+		return false
+	}
+	return errno == syscall.ERROR_ACCESS_DENIED
+}
+
+func isRetryableError(err error) bool {
+	if err == nil {
+		return false
+	}
+	path, errno := getPathFromError(err)
+	if path == "" {
+		return false
+	}
+	return errno == syscall.ERROR_ACCESS_DENIED || errno == windows.ERROR_SHARING_VIOLATION
+}
+
+func removeBlockingExe(blockingErr error) error {
+	path, _ := getPathFromError(blockingErr)
+	if path == "" {
+		return nil
+	}
+
+	// open handle for delete only
+	h, err := openDeleteHandle(path)
+	if err != nil {
+		return fmt.Errorf("failed to open handle for %q: %w", path, err)
+	}
+
+	// rename handle
+	err = renameHandle(h)
+	_ = windows.CloseHandle(h)
+	if err != nil {
+		return fmt.Errorf("failed to rename handle for %q: %w", path, err)
+	}
+
+	// re-open handle
+	h, err = openDeleteHandle(path)
+	if err != nil {
+		return fmt.Errorf("failed to open handle after rename for %q: %w", path, err)
+	}
+
+	// dispose of the handle
+	err = disposeHandle(h)
+	_ = windows.CloseHandle(h)
+	if err != nil {
+		return fmt.Errorf("failed to dispose handle for %q: %w", path, err)
+	}
+	return nil
+}
+
+func getPathFromError(blockingErr error) (string, syscall.Errno) {
+	var perr *fs.PathError
+	if errors.As(blockingErr, &perr) {
+		var errno syscall.Errno
+		if errors.As(perr.Err, &errno) {
+			return perr.Path, errno
+		}
+	}
+	return "", 0
+}
+
+func openDeleteHandle(path string) (windows.Handle, error) {
+	wPath, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return 0, err
+	}
+	handle, err := windows.CreateFile(
+		wPath,
+		windows.DELETE,
+		0,
+		nil,
+		windows.OPEN_EXISTING,
+		windows.FILE_ATTRIBUTE_NORMAL,
+		0,
+	)
+	if err != nil {
+		return 0, err
+	}
+	return handle, nil
+}
+
+func renameHandle(hHandle windows.Handle) error {
+	wRename, err := windows.UTF16FromString(":agentrm")
+	if err != nil {
+		return err
+	}
+
+	var rename fileRenameInfo
+	lpwStream := &wRename[0]
+	rename.FileNameLength = uint32(unsafe.Sizeof(lpwStream))
+
+	_, _, _ = windows.NewLazyDLL("kernel32.dll").NewProc("RtlCopyMemory").Call(
+		uintptr(unsafe.Pointer(&rename.FileName[0])),
+		uintptr(unsafe.Pointer(lpwStream)),
+		unsafe.Sizeof(lpwStream),
+	)
+
+	err = windows.SetFileInformationByHandle(
+		hHandle,
+		windows.FileRenameInfo,
+		(*byte)(unsafe.Pointer(&rename)),
+		uint32(unsafe.Sizeof(rename)+unsafe.Sizeof(lpwStream)),
+	)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+func disposeHandle(hHandle windows.Handle) error {
+	var deleteFile fileDispositionInfo
+	deleteFile.DeleteFile = true
+
+	err := windows.SetFileInformationByHandle(
+		hHandle,
+		windows.FileDispositionInfo,
+		(*byte)(unsafe.Pointer(&deleteFile)),
+		uint32(unsafe.Sizeof(deleteFile)),
+	)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+type fileRenameInfo struct {
+	Union struct {
+		ReplaceIfExists bool
+		Flags           uint32
+	}
+	RootDirectory  windows.Handle
+	FileNameLength uint32
+	FileName       [1]uint16
+}
+
+type fileDispositionInfo struct {
+	DeleteFile bool
+}

internal/pkg/agent/install/uninstall_windows_test.go

@@ -0,0 +1,55 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+//go:build windows
+
+package install
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+const simpleBlockForever = `
+package main
+
+import (
+    "math"
+    "time"
+)
+
+func main() {
+    <-time.After(time.Duration(math.MaxInt64))
+}
+`
+
+func TestRemovePath(t *testing.T) {
+	dir := filepath.Join(t.TempDir(), "subdir")
+	err := os.Mkdir(dir, 0644)
+	require.NoError(t, err)
+
+	src := filepath.Join(dir, "main.go")
+	err = os.WriteFile(src, []byte(simpleBlockForever), 0644)
+	require.NoError(t, err)
+
+	binary := filepath.Join(dir, "main.exe")
+	cmd := exec.Command("go", "build", "-o", binary, src)
+	_, err = cmd.CombinedOutput()
+	require.NoError(t, err)
+
+	cmd = exec.Command(binary)
+	err = cmd.Start()
+	require.NoError(t, err)
+	defer func() {
+		_ = cmd.Process.Kill()
+		_ = cmd.Wait()
+	}()
+
+	err = RemovePath(dir)
+	require.NoError(t, err)
+}