From bc946b870e871451f1a234b92a0069bd7d6874ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=96=E7=95=8C?= Date: Sun, 12 Jul 2026 14:49:49 +0800 Subject: [PATCH] Fix windivert open --- common/windivert/driver_windows.go | 60 +++++++++++++- common/windivert/handle_windows.go | 24 +----- common/windivert/integration_windows_test.go | 85 +++++++++++--------- 3 files changed, 107 insertions(+), 62 deletions(-) diff --git a/common/windivert/driver_windows.go b/common/windivert/driver_windows.go index 811cb03dd..34368ed8b 100644 --- a/common/windivert/driver_windows.go +++ b/common/windivert/driver_windows.go @@ -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 } diff --git a/common/windivert/handle_windows.go b/common/windivert/handle_windows.go index 800b431c0..bb99ce9f2 100644 --- a/common/windivert/handle_windows.go +++ b/common/windivert/handle_windows.go @@ -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 { diff --git a/common/windivert/integration_windows_test.go b/common/windivert/integration_windows_test.go index 100ee0d4b..750e57467 100644 --- a/common/windivert/integration_windows_test.go +++ b/common/windivert/integration_windows_test.go @@ -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)