Fix windivert open

2026-07-12 14:49:49 +08:00
parent 4bccd6fae1
commit bc946b870e
3 changed files with 107 additions and 62 deletions
@@ -26,6 +26,63 @@ const (
 // so Open can try CreateFile first and only install on FILE_NOT_FOUND.
 var driverDevName, _ = windows.UTF16PtrFromString(driverDeviceName)

+// acquireDevice opens the kernel device, installing the driver when it is
+// absent. The driver is marked for deletion at install time (see
+// installDriver), so it unloads once its last handle closes; the next Open
+// must reinstall it. When that Open races the still-in-progress unload,
+// CreateFile does not report a clean ERROR_FILE_NOT_FOUND — the
+// \\.\WinDivert symlink still resolves while the device object behind it is
+// torn down, so the open fails with ERROR_NO_SUCH_DEVICE. Treat every
+// "device not currently openable" code as a reinstall trigger and retry a
+// bounded number of times so the teardown of a prior instance settles.
+func acquireDevice() (windows.Handle, error) {
+	const maxRetries = 20
+	for retry := 0; ; retry++ {
+		device, err := openDevice()
+		if err == nil {
+			return device, nil
+		}
+		fatal := driverOpenFatal(err)
+		if fatal != nil {
+			return 0, fatal
+		}
+		err = installDriver()
+		if err != nil {
+			return 0, err
+		}
+		device, err = openDevice()
+		if err == nil {
+			return device, nil
+		}
+		fatal = driverOpenFatal(err)
+		if fatal != nil {
+			return 0, fatal
+		}
+		// Still absent right after a successful install: a prior instance's
+		// lingering device object shadows the freshly loaded one. Back off
+		// and retry the whole install/open.
+		if retry >= maxRetries {
+			return 0, E.Cause(err, "windivert: open device")
+		}
+		time.Sleep(50 * time.Millisecond)
+	}
+}
+
+// driverOpenFatal maps an openDevice failure to the error the caller should
+// surface, or nil when the failure means the driver is absent and a
+// (re)install should be attempted.
+func driverOpenFatal(err error) error {
+	if errors.Is(err, windows.ERROR_ACCESS_DENIED) {
+		return E.Cause(err, "windivert: open device (administrator required)")
+	}
+	if errors.Is(err, windows.ERROR_FILE_NOT_FOUND) ||
+		errors.Is(err, windows.ERROR_PATH_NOT_FOUND) ||
+		errors.Is(err, windows.ERROR_NO_SUCH_DEVICE) {
+		return nil
+	}
+	return E.Cause(err, "windivert: open device")
+}
+
 // Requires SeLoadDriverPrivilege (Administrator). Running the 386 build
 // under WOW64 on a 64-bit kernel is rejected — use the amd64 build.
 func installDriver() error {
@@ -81,7 +138,8 @@ func installDriver() error {
 			return nil
 		}
 		retryable := errors.Is(err, windows.ERROR_SERVICE_MARKED_FOR_DELETE) ||
-			errors.Is(err, windows.ERROR_SERVICE_DISABLED)
+			errors.Is(err, windows.ERROR_SERVICE_DISABLED) ||
+			errors.Is(err, windows.ERROR_OBJECT_ALREADY_EXISTS)
 		if !retryable || attempt >= 20 {
 			return err
 		}
@@ -51,29 +51,9 @@ func Open(filter *Filter, layer Layer, priority int16, flags Flag) (*Handle, err
 	if err != nil {
 		return nil, err
 	}
-	device, err := openDevice()
+	device, err := acquireDevice()
 	if err != nil {
-		if !errors.Is(err, windows.ERROR_FILE_NOT_FOUND) &&
-			!errors.Is(err, windows.ERROR_PATH_NOT_FOUND) {
-			if errors.Is(err, windows.ERROR_ACCESS_DENIED) {
-				return nil, E.Cause(err, "windivert: open device (administrator required)")
-			}
-			return nil, E.Cause(err, "windivert: open device")
-		}
-		// Device node missing: kernel driver not loaded. Install + retry.
-		// Matches WinDivertOpen's lazy-install path; avoids racing StartService
-		// against a still-loaded driver whose SCM record is marked for deletion.
-		err = installDriver()
-		if err != nil {
-			return nil, err
-		}
-		device, err = openDevice()
-		if err != nil {
-			if errors.Is(err, windows.ERROR_ACCESS_DENIED) {
-				return nil, E.Cause(err, "windivert: open device (administrator required)")
-			}
-			return nil, E.Cause(err, "windivert: open device")
-		}
+		return nil, err
 	}
 	event, err := windows.CreateEvent(nil, 1, 0, nil) // manual reset, unsignaled
 	if err != nil {
@@ -8,7 +8,6 @@ import (
 	"net/netip"
 	"os"
 	"path/filepath"
-	"strconv"
 	"testing"
 	"time"

@@ -91,55 +90,64 @@ func stopDriver(t *testing.T) {
 	serviceNameW, err := windows.UTF16PtrFromString(driverServiceName)
 	require.NoError(t, err)
 	service, err := windows.OpenService(manager, serviceNameW, windows.SERVICE_STOP|windows.SERVICE_QUERY_STATUS)
-	if errors.Is(err, windows.ERROR_SERVICE_DOES_NOT_EXIST) {
-		return
-	}
-	require.NoError(t, err)
-	defer windows.CloseServiceHandle(service)
-	var status windows.SERVICE_STATUS
-	err = windows.ControlService(service, windows.SERVICE_CONTROL_STOP, &status)
-	if err != nil &&
-		!errors.Is(err, windows.ERROR_SERVICE_NOT_ACTIVE) &&
-		!errors.Is(err, windows.ERROR_SERVICE_CANNOT_ACCEPT_CTRL) {
-		require.NoError(t, err)
+	if err == nil {
+		defer windows.CloseServiceHandle(service)
+		var status windows.SERVICE_STATUS
+		err = windows.ControlService(service, windows.SERVICE_CONTROL_STOP, &status)
+		if err != nil &&
+			!errors.Is(err, windows.ERROR_SERVICE_NOT_ACTIVE) &&
+			!errors.Is(err, windows.ERROR_SERVICE_CANNOT_ACCEPT_CTRL) {
+			require.NoError(t, err)
+		}
+		require.Eventually(t, func() bool {
+			queryErr := windows.QueryServiceStatus(service, &status)
+			return queryErr == nil && status.CurrentState == windows.SERVICE_STOPPED
+		}, 60*time.Second, 200*time.Millisecond, "driver did not reach SERVICE_STOPPED")
+	} else {
+		require.True(t, errors.Is(err, windows.ERROR_SERVICE_DOES_NOT_EXIST), "open driver service: %v", err)
 	}
+	// SCM can report SERVICE_STOPPED before the driver finishes deleting its
+	// device object. Wait for the absence acquireDevice uses to trigger install.
 	require.Eventually(t, func() bool {
-		queryErr := windows.QueryServiceStatus(service, &status)
-		return queryErr == nil && status.CurrentState == windows.SERVICE_STOPPED
-	}, 60*time.Second, 200*time.Millisecond, "driver did not reach SERVICE_STOPPED")
+		device, openErr := openDevice()
+		if openErr == nil {
+			_ = windows.CloseHandle(device)
+			return false
+		}
+		return errors.Is(openErr, windows.ERROR_FILE_NOT_FOUND) ||
+			errors.Is(openErr, windows.ERROR_PATH_NOT_FOUND) ||
+			errors.Is(openErr, windows.ERROR_NO_SUCH_DEVICE)
+	}, 60*time.Second, 200*time.Millisecond, "driver device remained openable after stop")
 }

 // The image lock on the cached .sys can outlive SERVICE_STOPPED by tens of
-// seconds (observed on GitHub-hosted runners), but it only blocks writes
-// and deletes — rename is permitted. Move the locked file aside instead of
-// waiting for the release.
-func plantTamperedDriver(t *testing.T, target string, planted []byte) {
+// seconds (observed on GitHub-hosted runners), and on current runner images
+// it blocks renames as well as writes and deletes. Tests that need to tamper
+// with the cache therefore redirect it to a directory the kernel has never
+// loaded a driver from. Not t.TempDir: once StartService maps a .sys from
+// the directory, the image lock makes the cleanup RemoveAll fail the test.
+func setTempDriverCache(t *testing.T) {
 	t.Helper()
-	require.NoError(t, os.MkdirAll(filepath.Dir(target), 0o755))
-	err := os.WriteFile(target, planted, 0o644)
-	if err == nil {
-		return
-	}
-	moved := target + ".locked-" + strconv.FormatInt(time.Now().UnixNano(), 10)
-	require.NoError(t, os.Rename(target, moved))
-	t.Cleanup(func() { os.Remove(moved) })
-	require.NoError(t, os.WriteFile(target, planted, 0o644))
+	dir, err := os.MkdirTemp("", "sing-box-windivert-test-")
+	require.NoError(t, err)
+	t.Cleanup(func() { os.RemoveAll(dir) })
+	t.Setenv("LocalAppData", dir)
 }

 // A foreign .sys planted in the user-writable cache must never reach
 // StartService: the install path has to detect the mismatch against the
 // embedded asset and repair the file before handing it to SCM.
 func TestIntegrationTamperedCacheRepaired(t *testing.T) {
-	// Open/close once so a working install is the baseline, then stop the
-	// driver so the cached file is writable for tampering.
-	h := openHandle(t, nil, FlagSendOnly)
-	require.NoError(t, h.Close())
+	setTempDriverCache(t)
+	// The driver left running by earlier tests would satisfy Open without
+	// touching the cache; stop it so the install path runs.
 	stopDriver(t)

 	target := cachedDriverPath(t)
-	plantTamperedDriver(t, target, []byte("planted payload, not the WinDivert driver"))
+	require.NoError(t, os.MkdirAll(filepath.Dir(target), 0o755))
+	require.NoError(t, os.WriteFile(target, []byte("planted payload, not the WinDivert driver"), 0o644))

-	h = openHandle(t, nil, FlagSendOnly)
+	h := openHandle(t, nil, FlagSendOnly)
 	require.NoError(t, h.Close())

 	content, err := os.ReadFile(target)
@@ -151,11 +159,10 @@ func TestIntegrationTamperedCacheRepaired(t *testing.T) {
 // install completes; without this, the file could be swapped between
 // verification and the kernel mapping it.
 func TestIntegrationDriverFileLockedWhileHeld(t *testing.T) {
-	target := cachedDriverPath(t)
-	// Stop the driver so the kernel image lock is gone and the failures
-	// asserted below can only come from the handle extractVerified holds.
-	stopDriver(t)
-	plantTamperedDriver(t, target, sysBytes)
+	// A fresh cache directory guarantees the failures asserted below can
+	// only come from the handle extractVerified holds, not a kernel image
+	// lock left by earlier tests.
+	setTempDriverCache(t)

 	sysPath, sysFile, err := extractVerified()
 	require.NoError(t, err)