From 363fa54601b84690537b7a6b9ccb6597001dd949 Mon Sep 17 00:00:00 2001 From: Reese Norris Date: Wed, 29 Jul 2026 11:59:00 -0400 Subject: [PATCH] fix(clientinject): Windows preflight no longer self-locks PE OpenFile then CreateFile(share=0) always hit ERROR_SHARING_VIOLATION in the same process, so Apply never ran on real Windows. Use path-only exclusive CreateFile on Windows; keep flock-based probe on Unix. --- internal/clientinject/preflight.go | 20 +++++------------- internal/clientinject/preflight_test.go | 19 +++++++++-------- internal/clientinject/preflight_unix.go | 19 +++++++++++++++++ internal/clientinject/preflight_windows.go | 24 ++++++++++++++-------- 4 files changed, 50 insertions(+), 32 deletions(-) diff --git a/internal/clientinject/preflight.go b/internal/clientinject/preflight.go index 85c6762..9534317 100644 --- a/internal/clientinject/preflight.go +++ b/internal/clientinject/preflight.go @@ -3,15 +3,15 @@ package clientinject import ( "errors" "fmt" - "os" "strings" ) // ErrClientRunning indicates the primary PE appears locked / in use. var ErrClientRunning = errors.New("clientinject: client appears to be running (file locked); quit the client completely, then Apply again") -// PreflightPrimaryPE tries to open the primary PE exclusively (O_RDWR + -// platform exclusive lock / CreateFile share-mode 0 on Windows). +// PreflightPrimaryPE tries to open the primary PE exclusively. +// On Windows this is CreateFileW with dwShareMode=0 (no prior open handle). +// On Unix this is O_RDWR + flock(LOCK_EX|LOCK_NB). // On lock/sharing failures it returns ErrClientRunning. // // This is a best-effort probe only: the exclusive lock is released before @@ -22,22 +22,12 @@ func PreflightPrimaryPE(path string) error { if path == "" { return fmt.Errorf("clientinject: preflight: empty primary PE path") } - f, err := os.OpenFile(path, os.O_RDWR, 0) - if err != nil { - if isLockError(err) { - return fmt.Errorf("%w: open %s: %v", ErrClientRunning, path, err) - } - return fmt.Errorf("clientinject: preflight open %s: %w", path, err) - } - defer f.Close() - - if err := tryExclusiveLock(f); err != nil { + if err := platformPreflightExclusive(path); err != nil { if isLockError(err) { return fmt.Errorf("%w: lock %s: %v", ErrClientRunning, path, err) } - return fmt.Errorf("clientinject: preflight lock %s: %w", path, err) + return fmt.Errorf("clientinject: preflight %s: %w", path, err) } - _ = unlockFile(f) return nil } diff --git a/internal/clientinject/preflight_test.go b/internal/clientinject/preflight_test.go index 90bd9b7..2f689c3 100644 --- a/internal/clientinject/preflight_test.go +++ b/internal/clientinject/preflight_test.go @@ -36,25 +36,28 @@ func TestPreflightPrimaryPE_Empty(t *testing.T) { } func TestPreflightPrimaryPE_Locked(t *testing.T) { - if runtime.GOOS == "windows" { - // Windows LockFileEx/share semantics differ; exercise best-effort path only. - t.Log("windows: lock contention test is best-effort; skipping strict assertion") - } dir := t.TempDir() path := filepath.Join(dir, "app.exe") if err := os.WriteFile(path, []byte("MZ\x00\x00"), 0o644); err != nil { t.Fatal(err) } - // Hold exclusive flock in this process. + // Hold an open handle so exclusive re-open / flock fails. holder, err := os.OpenFile(path, os.O_RDWR, 0) if err != nil { t.Fatal(err) } defer holder.Close() - if err := tryExclusiveLock(holder); err != nil { - t.Fatalf("hold lock: %v", err) + + if runtime.GOOS != "windows" { + // Unix: need flock on the holder; open alone does not block another open. + if err := tryExclusiveLock(holder); err != nil { + t.Fatalf("hold lock: %v", err) + } + defer unlockFile(holder) } - defer unlockFile(holder) + // Windows: os.OpenFile hold is enough — CreateFile(share=0) fails while any + // handle is open (including this process). Do not OpenFile+CreateFile exclusive + // in the same preflight path (that always self-conflicts). err = PreflightPrimaryPE(path) if err == nil { diff --git a/internal/clientinject/preflight_unix.go b/internal/clientinject/preflight_unix.go index 992c241..0e4b711 100644 --- a/internal/clientinject/preflight_unix.go +++ b/internal/clientinject/preflight_unix.go @@ -8,6 +8,25 @@ import ( "syscall" ) +// platformPreflightExclusive opens the path O_RDWR and takes a non-blocking +// exclusive flock, then releases both. +func platformPreflightExclusive(path string) error { + f, err := os.OpenFile(path, os.O_RDWR, 0) + if err != nil { + if isLockError(err) { + return fmt.Errorf("%w: open %s: %v", ErrClientRunning, path, err) + } + return fmt.Errorf("open %s: %w", path, err) + } + defer f.Close() + + if err := tryExclusiveLock(f); err != nil { + return err + } + _ = unlockFile(f) + return nil +} + func tryExclusiveLock(f *os.File) error { err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) if err != nil { diff --git a/internal/clientinject/preflight_windows.go b/internal/clientinject/preflight_windows.go index 67515e4..dc2db6c 100644 --- a/internal/clientinject/preflight_windows.go +++ b/internal/clientinject/preflight_windows.go @@ -15,8 +15,10 @@ import ( // running EXEs usually do not hold range locks — LockFileEx can succeed while // the client is still running. // -// We always attempt CreateFile exclusive. LockFileEx is a secondary signal only -// (returns ErrClientRunning when it reports a lock/sharing violation). +// Important: do NOT open the path with os.OpenFile before CreateFile exclusive. +// A same-process open handle makes CreateFile(share=0) always fail with +// ERROR_SHARING_VIOLATION (confirmed on Windows 10/11). Path-based exclusive +// open is the only correct preflight pattern on Windows. var ( modkernel32 = syscall.NewLazyDLL("kernel32.dll") @@ -38,12 +40,17 @@ const ( _ERROR_SHARING_VIOLATION = 32 ) +// platformPreflightExclusive is the Windows "client running" probe: CreateFile +// with share mode 0, then close. No long-lived handle. +func platformPreflightExclusive(path string) error { + return tryCreateFileExclusive(path) +} + +// tryExclusiveLock is used by tests that already hold an *os.File. +// Prefer platformPreflightExclusive for production preflight. +// CreateFile exclusive is attempted only after the caller's handle would +// conflict — so this path uses LockFileEx on the existing handle only. func tryExclusiveLock(f *os.File) error { - // Primary gate: CreateFile with share mode 0 (always). - if err := tryCreateFileExclusive(f.Name()); err != nil { - return err - } - // Secondary: LockFileEx on the already-open Go handle (best-effort). var ol syscall.Overlapped r1, _, e1 := procLockFileEx.Call( f.Fd(), @@ -59,8 +66,7 @@ func tryExclusiveLock(f *os.File) error { errno == syscall.Errno(_ERROR_SHARING_VIOLATION) { return fmt.Errorf("%w: LockFileEx: %v", ErrClientRunning, errno) } - // Non-lock LockFileEx failure after CreateFile exclusive succeeded: - // treat as probe noise (CreateFile already proved exclusive open). + // Non-lock LockFileEx failure: treat as probe noise. return nil } return nil