diff --git a/AGENTS.md b/AGENTS.md index 12f22ca..ac7ff11 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,6 +32,7 @@ Operational rules for agents and humans changing this repository. This file is t | `internal/clientinject` | Engine plan/apply/revert | Must not import server/web/afv/db/postoffice/session/cluster/sweatbox/metar/auth/serviceapi | | `internal/clientinject/cilus` | Pure CLR `#US` encode/decode | **stdlib only** | | `internal/clientinject/vpilotconfig` | Pure vPilot 3DES config crypto + XML | **stdlib only** | +| `internal/clientinject/pepatch` | PE/binary overwrite + padded string + UTF-16 scan | **stdlib only** | **Wire format:** `pkg/protocol` (FSD) and `pkg/twrfiles` (.apt/.air) — no alternate field order or marshaling in handlers/clients. @@ -60,7 +61,8 @@ internal/auth → protocol internal/session → protocol internal/clientinject/cilus → (stdlib only) internal/clientinject/vpilotconfig → (stdlib only) -internal/clientinject → cilus, vpilotconfig, …; never server/web/afv/db/… +internal/clientinject/pepatch → (stdlib only) +internal/clientinject → cilus, vpilotconfig, pepatch, …; never server/web/afv/db/… cmd/openfsd-client → internal/clientinject (+ GUI); never server/web/afv/db/… ``` @@ -88,8 +90,9 @@ Enforce with `scripts/check-import-graph.sh`. | `internal/clientinject` (incl. subpackages) | `server`, `web`, `afv`, `db`, `postoffice`, `session`, `cluster`, `sweatbox`, `metar`, `auth`, `serviceapi` | | `internal/clientinject/cilus` | Any non-stdlib import | | `internal/clientinject/vpilotconfig` | Any non-stdlib import | +| `internal/clientinject/pepatch` | Any non-stdlib import | -Stdlib heuristic: first path element contains no `.` (e.g. `fmt`, `net/http`). Third-party is never allowed in `pkg/protocol`, `pkg/twrfiles`, `internal/geo`, `internal/clientinject/cilus`, or `internal/clientinject/vpilotconfig`. +Stdlib heuristic: first path element contains no `.` (e.g. `fmt`, `net/http`). Third-party is never allowed in `pkg/protocol`, `pkg/twrfiles`, `internal/geo`, `internal/clientinject/cilus`, `internal/clientinject/vpilotconfig`, or `internal/clientinject/pepatch`. **Cycle rule:** `session` never imports `postoffice`. Postoffice depends on a narrow participant/send port. Shared errors like `ErrCallsignInUse` live next to the registry, not in `pkg/protocol`. @@ -191,6 +194,8 @@ Enforced by `scripts/check-coverage.sh` (CI). | `internal/cluster` | ≥90% | **Hard** | | `internal/clientinject/cilus` | ≥98% | **Hard** | | `internal/clientinject/vpilotconfig` | ≥98% | **Hard** | +| `internal/clientinject/pepatch` | ≥95% | **Hard** | +| `internal/clientinject` | ≥85% | Soft (report only; hard after GUI/adapters mature) | | `internal/web` | ≥80% | Soft (report only) | | `internal/afv` | ≥80% | Soft (P0; hard ≥85 later) | | Overall aspirational | 90% | Soft (report only) | diff --git a/internal/clientinject/backup.go b/internal/clientinject/backup.go index 652b7a5..630ef1b 100644 --- a/internal/clientinject/backup.go +++ b/internal/clientinject/backup.go @@ -2,6 +2,7 @@ package clientinject import ( "encoding/json" + "errors" "fmt" "path/filepath" "time" @@ -21,6 +22,12 @@ const ( ManifestStatusReverted = "reverted" ) +// ErrPriorInjectActive is returned when Apply finds an unfinished or applied +// inject that must be Reverted first (or when bak would be clobbered). +// For status=applied, re-Apply is allowed if stock bak is preserved; this +// error is used for status=in_progress (incomplete prior Apply). +var ErrPriorInjectActive = errors.New("clientinject: prior inject in progress; Revert first, then Apply again") + // Manifest records a transactional Apply for Revert and recovery. type Manifest struct { ClientID string `json:"client_id"` @@ -102,7 +109,9 @@ func CollectPlanTargets(plan *Plan) []string { return out } -// CreateBackups copies each existing target to its .openfsd-bak sibling. +// CreateBackups ensures each existing target has a .openfsd-bak sibling. +// Existing bak files are never overwritten — they hold stock content from the +// first Apply and must remain restorable across re-Apply. // Missing targets are skipped (they may be created by Apply). func CreateBackups(w FileWriter, targets []string) ([]ManifestFile, error) { var files []ManifestFile @@ -112,6 +121,11 @@ func CreateBackups(w FileWriter, targets []string) ([]ManifestFile, error) { continue } bak := BackupPath(orig) + if _, err := w.Stat(bak); err == nil { + // Preserve existing stock bak. + files = append(files, ManifestFile{Original: orig, Backup: bak}) + continue + } if err := w.CopyFile(orig, bak); err != nil { return files, fmt.Errorf("clientinject: backup %s: %w", orig, err) } @@ -208,3 +222,22 @@ func NewManifest(plan *Plan, files []ManifestFile, preflightOK bool) *Manifest { InstallRoot: plan.Install.RootDir, } } + +// CheckPriorInject refuses Apply when a prior inject is incomplete (in_progress). +// status=applied / failed / reverted is OK: CreateBackups preserves stock bak. +// Recovery: Revert uses bak even if status is still in_progress. +func CheckPriorInject(w FileWriter, installRoot string) error { + path := ManifestPath(installRoot) + if _, err := w.Stat(path); err != nil { + return nil // no manifest + } + m, err := ReadManifest(w, installRoot) + if err != nil { + return fmt.Errorf("clientinject: read prior manifest: %w", err) + } + if m.Status == ManifestStatusInProgress { + return fmt.Errorf("%w (status=%s); Revert restores .openfsd-bak even if status is in_progress", + ErrPriorInjectActive, m.Status) + } + return nil +} diff --git a/internal/clientinject/doc.go b/internal/clientinject/doc.go index 22baf67..4ebdfcc 100644 --- a/internal/clientinject/doc.go +++ b/internal/clientinject/doc.go @@ -4,7 +4,8 @@ // // Pure subpackages (stdlib only): // - pepatch — PE/binary overwrite helpers -// - cilus / vpilotconfig — land in sibling PRs +// - cilus — CLR #US heap encode/decode +// - vpilotconfig — vPilot 3DES config crypto + XML field rewrite // // This package must not import server, web, afv, db, postoffice, session, // cluster, sweatbox, metar, auth, or serviceapi. diff --git a/internal/clientinject/engine.go b/internal/clientinject/engine.go index b5becb2..b1dfb69 100644 --- a/internal/clientinject/engine.go +++ b/internal/clientinject/engine.go @@ -87,9 +87,18 @@ func fileSHA1(w FileWriter, path string) (string, error) { return hex.EncodeToString(sum[:]), nil } +// normalizeInstall makes PrimaryPE absolute under RootDir when relative. +func normalizeInstall(install Install) Install { + if pe := AbsPrimaryPE(install); pe != "" { + install.PrimaryPE = pe + } + return install +} + // Plan runs adapter.Plan after Verify. Does not write disk. func (e *Engine) Plan(ctx context.Context, install Install, ep Endpoints) (*Plan, error) { _ = ctx + install = normalizeInstall(install) a, ok := e.Adapters[install.ClientID] if !ok { return nil, fmt.Errorf("clientinject: no adapter for client %q", install.ClientID) @@ -124,6 +133,13 @@ func (e *Engine) Plan(ctx context.Context, install Install, ep Endpoints) (*Plan // Apply runs preflight, plan blockers check, backup, adapter.Apply, healthcheck // with automatic restore on failure. +// +// Preflight is a probe only (lock released before mutations). PrimaryPE is +// normalized via AbsPrimaryPE. Existing .openfsd-bak files are never clobbered +// so re-Apply keeps stock for Revert. Incomplete prior inject (manifest +// status=in_progress) must be Reverted first — Revert uses bak even if status +// is still in_progress (e.g. finalize WriteManifest failed after a successful +// patch). func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) { if plan == nil { return nil, fmt.Errorf("clientinject: nil plan") @@ -131,18 +147,61 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) { if len(plan.Blockers) > 0 { return nil, fmt.Errorf("clientinject: plan has blockers: %s", strings.Join(plan.Blockers, "; ")) } + // Normalize PrimaryPE to absolute under RootDir for preflight + bak paths. + plan.Install = normalizeInstall(plan.Install) + a, ok := e.Adapters[plan.Install.ClientID] if !ok { return nil, fmt.Errorf("clientinject: no adapter for client %q", plan.Install.ClientID) } w := e.writer() - // Preflight: exclusive open on primary PE. - preflightOK := true + // Refuse incomplete prior inject (status=in_progress). Applied/failed/reverted OK. + if err := CheckPriorInject(w, plan.Install.RootDir); err != nil { + return nil, err + } + + // Re-resolve profile + Verify at Apply entry (Plan may be stale / caller-built). + profile, err := e.ResolveProfile(plan.Install) + if err != nil { + return nil, err + } + if plan.Install.ProfileID == "" { + plan.Install.ProfileID = profile.ProfileID + } + // On first apply (no stock bak for PE yet), PE must match profile stock SHA. + // Re-apply after a prior successful inject keeps stock in .openfsd-bak while + // the live PE may already be patched — skip live-hash stock check then. + if plan.Install.PrimaryPE != "" { + want := strings.ToLower(strings.TrimSpace(profile.PrimaryBinary.SHA1)) + bak := BackupPath(plan.Install.PrimaryPE) + if _, bakErr := w.Stat(bak); bakErr != nil && want != "" { + sum, err := fileSHA1(w, plan.Install.PrimaryPE) + if err != nil { + return nil, fmt.Errorf("clientinject: hash PE for verify: %w", err) + } + if !strings.EqualFold(sum, want) { + return nil, fmt.Errorf("clientinject: PE sha1 %s does not match profile %s stock %s", + sum, profile.ProfileID, want) + } + plan.Install.HashSHA1 = sum + } else if plan.Install.HashSHA1 == "" { + if sum, err := fileSHA1(w, plan.Install.PrimaryPE); err == nil { + plan.Install.HashSHA1 = sum + } + } + } + if err := a.Verify(plan.Install, profile); err != nil { + return nil, fmt.Errorf("clientinject: verify: %w", err) + } + + // Preflight: exclusive open probe on absolute primary PE (lock not held across Apply). + preflightOK := false if plan.Install.PrimaryPE != "" { if err := PreflightPrimaryPE(plan.Install.PrimaryPE); err != nil { return nil, err } + preflightOK = true } targets := CollectPlanTargets(plan) @@ -152,8 +211,8 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) { } manifest := NewManifest(plan, files, preflightOK) if err := WriteManifest(w, manifest); err != nil { - // Best-effort restore if we already wrote bak. - _ = RestoreBackups(w, files) + // Best-effort restore only for newly created baks is complex; leave baks. + // Disk originals unchanged at this point (adapter not yet called). return nil, err } @@ -165,17 +224,19 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) { if rerr := RestoreBackups(w, files); rerr != nil { slog.Error("clientinject restore after apply failure also failed", "err", rerr) manifest.Status = ManifestStatusFailed + manifest.MutationsApplied = nil manifest.Error = fmt.Sprintf("apply: %v; restore: %v", applyErr, rerr) _ = WriteManifest(w, manifest) return nil, fmt.Errorf("clientinject: apply failed (%w) and restore failed (%v)", applyErr, rerr) } manifest.Status = ManifestStatusFailed + manifest.MutationsApplied = nil manifest.Error = applyErr.Error() _ = WriteManifest(w, manifest) return nil, fmt.Errorf("clientinject: apply failed (restored from bak): %w", applyErr) } - // Record applied mutation IDs. + // Record applied mutation IDs (cleared again if healthcheck reverts). applied := make([]string, 0, len(plan.Mutations)) for _, m := range plan.Mutations { if m.ID != "" { @@ -194,7 +255,9 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) { _ = WriteManifest(w, manifest) return nil, fmt.Errorf("clientinject: healthcheck failed (%w) and restore failed (%v)", err, rerr) } + // Disk is stock again — do not claim mutations applied. manifest.Status = ManifestStatusFailed + manifest.MutationsApplied = nil manifest.Error = "healthcheck: " + err.Error() _ = WriteManifest(w, manifest) return nil, fmt.Errorf("clientinject: healthcheck failed (restored from bak): %w", err) @@ -203,7 +266,11 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) { manifest.Status = ManifestStatusApplied manifest.Error = "" if err := WriteManifest(w, manifest); err != nil { - return nil, fmt.Errorf("clientinject: finalize manifest: %w", err) + // PE/config remain patched; bak still holds stock. Manifest may stay + // in_progress — next Apply is refused until Revert (uses bak). + slog.Error("clientinject finalize manifest failed; disk patched, bak stock intact — Revert to recover", + "err", err, "install_root", plan.Install.RootDir) + return nil, fmt.Errorf("clientinject: finalize manifest: %w (disk may be patched; Revert restores from .openfsd-bak)", err) } return &ApplyResult{ @@ -214,6 +281,8 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) { } // Revert restores bak files from the install root manifest. +// After restore, if the primary PE path is known and a profile SHA-1 is on the +// manifest, a soft mismatch is logged (does not fail Revert). func (e *Engine) Revert(ctx context.Context, installRoot string) error { _ = ctx if installRoot == "" { @@ -230,8 +299,45 @@ func (e *Engine) Revert(ctx context.Context, installRoot string) error { if err := RestoreBackups(w, m.Files); err != nil { return err } + + // Soft stock-SHA check when profile is available. + if e.Profiles != nil && m.ProfileID != "" { + if p, ok := e.Profiles.Get(m.ProfileID); ok { + want := strings.ToLower(strings.TrimSpace(p.PrimaryBinary.SHA1)) + // Find primary PE among restored files (first matching relative name) or Absolute. + pePath := "" + if p.PrimaryBinary.RelativePath != "" { + cand := filepath.Join(installRoot, p.PrimaryBinary.RelativePath) + for _, f := range m.Files { + if filepath.Clean(f.Original) == filepath.Clean(cand) { + pePath = f.Original + break + } + } + if pePath == "" { + // Fallback: any original ending with relative path. + for _, f := range m.Files { + if strings.HasSuffix(filepath.Clean(f.Original), filepath.Clean(p.PrimaryBinary.RelativePath)) { + pePath = f.Original + break + } + } + } + } + if pePath != "" && want != "" { + if sum, err := fileSHA1(w, pePath); err == nil { + if !strings.EqualFold(sum, want) { + slog.Warn("clientinject revert: primary PE sha1 != profile stock after restore", + "path", pePath, "got", sum, "want", want, "profile_id", m.ProfileID) + } + } + } + } + } + m.Status = ManifestStatusReverted m.Error = "" + m.MutationsApplied = nil if err := WriteManifest(w, m); err != nil { return err } diff --git a/internal/clientinject/engine_test.go b/internal/clientinject/engine_test.go index 258a6b2..6f6ed5d 100644 --- a/internal/clientinject/engine_test.go +++ b/internal/clientinject/engine_test.go @@ -405,6 +405,282 @@ func TestCollectPlanTargets(t *testing.T) { } } +func TestEngine_ReApplyPreservesStockBak(t *testing.T) { + // Apply → re-Apply with different payload → Revert must restore original stock. + root, pe, payload, stock := setupFakeInstall(t) + store := NewProfileStore() + _ = store.Add(&Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: PrimaryBinarySpec{ + RelativePath: "app.exe", + SHA1: sha1hex([]byte("MZ-fake-pe")), + }, + }) + fake := &FakeAdapter{ID: "fake", NewData: []byte("PATCH-V1")} + eng := NewEngine(store, fake) + install := Install{ + ClientID: "fake", + RootDir: root, + PrimaryPE: pe, + HashSHA1: sha1hex([]byte("MZ-fake-pe")), + ProfileID: "fake-1", + } + plan, err := eng.Plan(context.Background(), install, Endpoints{WebBaseURL: "https://x.test"}) + if err != nil { + t.Fatal(err) + } + if _, err := eng.Apply(context.Background(), plan); err != nil { + t.Fatal(err) + } + // Stock bak must still be original after first apply. + bak1, err := os.ReadFile(payload + BackupSuffix) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(bak1, stock) { + t.Fatalf("first bak=%q stock=%q", bak1, stock) + } + // Re-apply different content; bak must not be clobbered with PATCH-V1. + fake.NewData = []byte("PATCH-V2") + plan2, err := eng.Plan(context.Background(), install, Endpoints{WebBaseURL: "https://y.test"}) + if err != nil { + t.Fatal(err) + } + // Live PE is still stock (fake only patches payload.bin), so Plan/Verify OK. + if _, err := eng.Apply(context.Background(), plan2); err != nil { + t.Fatal(err) + } + bak2, err := os.ReadFile(payload + BackupSuffix) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(bak2, stock) { + t.Fatalf("re-apply clobbered bak: got %q want stock %q", bak2, stock) + } + got, _ := os.ReadFile(payload) + if !bytes.Equal(got, []byte("PATCH-V2")) { + t.Fatalf("payload=%q", got) + } + if err := eng.Revert(context.Background(), root); err != nil { + t.Fatal(err) + } + got, _ = os.ReadFile(payload) + if !bytes.Equal(got, stock) { + t.Fatalf("after revert got %q want stock", got) + } +} + +func TestEngine_RelativePrimaryPE(t *testing.T) { + root, _, payload, stock := setupFakeInstall(t) + store := NewProfileStore() + _ = store.Add(&Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))}, + }) + fake := &FakeAdapter{ID: "fake", NewData: []byte("REL-PATCH")} + eng := NewEngine(store, fake) + install := Install{ + ClientID: "fake", + RootDir: root, + PrimaryPE: "app.exe", // relative + ProfileID: "fake-1", + HashSHA1: sha1hex([]byte("MZ-fake-pe")), + } + plan, err := eng.Plan(context.Background(), install, Endpoints{}) + if err != nil { + t.Fatal(err) + } + if !filepath.IsAbs(plan.Install.PrimaryPE) { + t.Fatalf("Plan should abs PrimaryPE, got %q", plan.Install.PrimaryPE) + } + if _, err := eng.Apply(context.Background(), plan); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(payload) + if !bytes.Equal(got, []byte("REL-PATCH")) { + t.Fatalf("%q", got) + } + // Revert still works + if err := eng.Revert(context.Background(), root); err != nil { + t.Fatal(err) + } + got, _ = os.ReadFile(payload) + if !bytes.Equal(got, stock) { + t.Fatalf("revert %q", got) + } +} + +func TestEngine_PriorInjectInProgressRefuses(t *testing.T) { + root, pe, _, _ := setupFakeInstall(t) + store := NewProfileStore() + _ = store.Add(&Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))}, + }) + // Stale in_progress manifest (e.g. finalize WriteManifest failed after patch). + m := &Manifest{ + InstallRoot: root, + Status: ManifestStatusInProgress, + ClientID: "fake", + ProfileID: "fake-1", + Files: []ManifestFile{{Original: pe, Backup: BackupPath(pe)}}, + } + if err := WriteManifest(OSFileWriter{}, m); err != nil { + t.Fatal(err) + } + fake := &FakeAdapter{ID: "fake"} + eng := NewEngine(store, fake) + plan := &Plan{ + Install: Install{ClientID: "fake", RootDir: root, PrimaryPE: pe, ProfileID: "fake-1", HashSHA1: sha1hex([]byte("MZ-fake-pe"))}, + Mutations: []Mutation{{ID: "x", TargetRel: "payload.bin", Detail: WriteFileDetail{Contents: []byte("x")}}}, + } + _, err := eng.Apply(context.Background(), plan) + if err == nil || !errors.Is(err, ErrPriorInjectActive) { + t.Fatalf("err=%v", err) + } +} + +func TestEngine_ApplyVerifyMismatch(t *testing.T) { + root, pe, _, _ := setupFakeInstall(t) + // Wrong PE contents vs profile stock SHA. + if err := os.WriteFile(pe, []byte("WRONG-PE"), 0o644); err != nil { + t.Fatal(err) + } + store := NewProfileStore() + _ = store.Add(&Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))}, + }) + fake := &FakeAdapter{ID: "fake"} + eng := NewEngine(store, fake) + plan := &Plan{ + Install: Install{ClientID: "fake", RootDir: root, PrimaryPE: pe, ProfileID: "fake-1"}, + Mutations: []Mutation{{ID: "x", TargetRel: "payload.bin", Detail: WriteFileDetail{Contents: []byte("x")}}}, + } + _, err := eng.Apply(context.Background(), plan) + if err == nil { + t.Fatal("expected sha mismatch") + } +} + +func TestEngine_HealthFailClearsMutationsApplied(t *testing.T) { + root, pe, _, _ := setupFakeInstall(t) + store := NewProfileStore() + _ = store.Add(&Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))}, + }) + fake := &FakeAdapter{ID: "fake", NewData: []byte("BAD"), FailHealth: true} + eng := NewEngine(store, fake) + install := Install{ClientID: "fake", RootDir: root, PrimaryPE: pe, ProfileID: "fake-1", HashSHA1: sha1hex([]byte("MZ-fake-pe"))} + plan, err := eng.Plan(context.Background(), install, Endpoints{}) + if err != nil { + t.Fatal(err) + } + _, err = eng.Apply(context.Background(), plan) + if err == nil { + t.Fatal("expected health error") + } + m, err := ReadManifest(OSFileWriter{}, root) + if err != nil { + t.Fatal(err) + } + if len(m.MutationsApplied) != 0 { + t.Fatalf("MutationsApplied should be cleared after health restore, got %v", m.MutationsApplied) + } +} + +func TestEngine_PreflightOKFalseWhenNoPE(t *testing.T) { + root := t.TempDir() + payload := filepath.Join(root, "payload.bin") + stock := []byte("stock") + if err := os.WriteFile(payload, stock, 0o644); err != nil { + t.Fatal(err) + } + store := NewProfileStore() + _ = store.Add(&Profile{ + SchemaVersion: 2, + ProfileID: "fake-1", + ClientID: "fake", + PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe"}, // empty sha — no stock check + }) + fake := &FakeAdapter{ID: "fake", NewData: []byte("P")} + eng := NewEngine(store, fake) + plan := &Plan{ + Install: Install{ClientID: "fake", RootDir: root, PrimaryPE: "", ProfileID: "fake-1"}, + Mutations: []Mutation{{ID: "write_payload", TargetRel: "payload.bin", Detail: WriteFileDetail{Contents: []byte("P")}}}, + } + res, err := eng.Apply(context.Background(), plan) + if err != nil { + t.Fatal(err) + } + _ = res + m, _ := ReadManifest(OSFileWriter{}, root) + if m.PreflightOK { + t.Fatal("preflight_ok should be false when PrimaryPE empty") + } +} + +func TestCreateBackups_PreservesExisting(t *testing.T) { + w := OSFileWriter{} + dir := t.TempDir() + orig := filepath.Join(dir, "f.bin") + bak := BackupPath(orig) + if err := w.WriteFile(orig, []byte("LIVE")); err != nil { + t.Fatal(err) + } + if err := w.WriteFile(bak, []byte("STOCK")); err != nil { + t.Fatal(err) + } + files, err := CreateBackups(w, []string{orig}) + if err != nil { + t.Fatal(err) + } + if len(files) != 1 { + t.Fatalf("%v", files) + } + got, _ := w.ReadFile(bak) + if string(got) != "STOCK" { + t.Fatalf("clobbered bak: %q", got) + } +} + +func TestFileWriter_WriteFilePreservesMode(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "x.bin") + if err := os.WriteFile(path, []byte("a"), 0o755); err != nil { + t.Fatal(err) + } + w := OSFileWriter{} + if err := w.WriteFile(path, []byte("bb")); err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm()&0o111 == 0 { + // On some FS execute bit may not stick; at least we tried 0o755. + // Require that we didn't force only 0o644 if OS preserves. + t.Logf("mode=%v (execute may be unsupported on this FS)", info.Mode()) + } + // Content updated + got, _ := os.ReadFile(path) + if string(got) != "bb" { + t.Fatalf("%q", got) + } +} + func TestEngine_RegisterAdapterAndWriterNil(t *testing.T) { eng := &Engine{Profiles: NewProfileStore()} fake := &FakeAdapter{ID: "fake"} diff --git a/internal/clientinject/filewriter.go b/internal/clientinject/filewriter.go index 01bc3a4..25c8e16 100644 --- a/internal/clientinject/filewriter.go +++ b/internal/clientinject/filewriter.go @@ -36,8 +36,18 @@ func (OSFileWriter) ReadFile(path string) ([]byte, error) { } // WriteFile implements FileWriter. +// When overwriting an existing file, preserves its permission bits (so PE +// rewrites via full-file WriteFile keep the executable bit on Unix/Wine). +// New files default to 0o644. Prefer OpenReadWrite / pepatch for PE mutations. func (OSFileWriter) WriteFile(path string, data []byte) error { - return os.WriteFile(path, data, 0o644) + mode := fs.FileMode(0o644) + if info, err := os.Stat(path); err == nil { + mode = info.Mode().Perm() + if mode == 0 { + mode = 0o644 + } + } + return os.WriteFile(path, data, mode) } // CopyFile implements FileWriter. diff --git a/internal/clientinject/pepatch/pepatch.go b/internal/clientinject/pepatch/pepatch.go index 338958f..1415e9f 100644 --- a/internal/clientinject/pepatch/pepatch.go +++ b/internal/clientinject/pepatch/pepatch.go @@ -3,6 +3,7 @@ package pepatch import ( + "bytes" "encoding/binary" "errors" "fmt" @@ -129,21 +130,9 @@ func ScanUTF16String(data []byte, s string) []int64 { var hits []int64 // Byte-aligned scan; PE strings are 2-byte aligned but residual scan is exhaustive. for i := 0; i+need <= len(data); i++ { - if bytesEqual(data[i:i+need], pat) { + if bytes.Equal(data[i:i+need], pat) { hits = append(hits, int64(i)) } } return hits } - -func bytesEqual(a, b []byte) bool { - if len(a) != len(b) { - return false - } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true -} diff --git a/internal/clientinject/pepatch/pepatch_test.go b/internal/clientinject/pepatch/pepatch_test.go index 5ac94f6..1b47aac 100644 --- a/internal/clientinject/pepatch/pepatch_test.go +++ b/internal/clientinject/pepatch/pepatch_test.go @@ -246,18 +246,6 @@ func TestScanUTF16String_TooShort(t *testing.T) { } } -func TestBytesEqual(t *testing.T) { - if bytesEqual([]byte{1}, []byte{1, 2}) { - t.Fatal("len") - } - if !bytesEqual([]byte{1, 2}, []byte{1, 2}) { - t.Fatal("eq") - } - if bytesEqual([]byte{1, 2}, []byte{1, 3}) { - t.Fatal("ne") - } -} - func TestEncodePadded_UTF16Alias(t *testing.T) { // via PaddedStringOverwrite encoding aliases var buf seekBuffer diff --git a/internal/clientinject/preflight.go b/internal/clientinject/preflight.go index fa9890b..85c6762 100644 --- a/internal/clientinject/preflight.go +++ b/internal/clientinject/preflight.go @@ -11,9 +11,13 @@ import ( 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). On lock/sharing failures it returns -// ErrClientRunning. Best-effort: process-list detection is optional and not -// required for the gate. +// platform exclusive lock / CreateFile share-mode 0 on Windows). +// On lock/sharing failures it returns ErrClientRunning. +// +// This is a best-effort probe only: the exclusive lock is released before +// return. Engine.Apply does not hold the lock across CreateBackups/adapter +// mutations (TOCTOU is possible if the client starts mid-Apply). Process-list +// detection is optional and not required for the gate. func PreflightPrimaryPE(path string) error { if path == "" { return fmt.Errorf("clientinject: preflight: empty primary PE path") diff --git a/internal/clientinject/preflight_windows.go b/internal/clientinject/preflight_windows.go index c7f6a8c..67515e4 100644 --- a/internal/clientinject/preflight_windows.go +++ b/internal/clientinject/preflight_windows.go @@ -9,11 +9,14 @@ import ( "unsafe" ) -// On Windows, a second CreateFile with share-mode 0 fails if the PE is mapped -// or open without share. We re-open via CreateFileW with dwShareMode=0. -// If that fails with sharing violation, treat as client running. -// Holding a simple *os.File from OpenFile may still allow another OpenFile -// (Go uses FILE_SHARE_READ|WRITE); exclusive CreateFile is the real gate. +// On Windows, CreateFileW with dwShareMode=0 is the real "client running" gate: +// a mapped/open PE typically fails exclusive open with ERROR_SHARING_VIOLATION +// even when no byte-range locks exist. LockFileEx alone is insufficient because +// 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). var ( modkernel32 = syscall.NewLazyDLL("kernel32.dll") @@ -36,7 +39,11 @@ const ( ) func tryExclusiveLock(f *os.File) error { - // Prefer LockFileEx exclusive non-blocking on the already-open handle. + // 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(), @@ -52,8 +59,9 @@ func tryExclusiveLock(f *os.File) error { errno == syscall.Errno(_ERROR_SHARING_VIOLATION) { return fmt.Errorf("%w: LockFileEx: %v", ErrClientRunning, errno) } - // Fallback: try CreateFile with share mode 0. - return tryCreateFileExclusive(f.Name()) + // Non-lock LockFileEx failure after CreateFile exclusive succeeded: + // treat as probe noise (CreateFile already proved exclusive open). + return nil } return nil } diff --git a/scripts/check-client-profiles-sync.sh b/scripts/check-client-profiles-sync.sh index 41bde90..1fef098 100755 --- a/scripts/check-client-profiles-sync.sh +++ b/scripts/check-client-profiles-sync.sh @@ -54,10 +54,7 @@ for emb in "$EMBED"/*.yaml "$EMBED"/*.yml; do echo " FAIL: $base client_id embed=$ec mirror=$mc" failed=1 fi - # primary_binary.sha1 appears after primary_binary: — use simple grep of sha1 under file - esha="$(grep -E '^\s+sha1:' "$emb" 2>/dev/null | head -1 | sed -E 's/.*sha1:[[:space:]]*//' | tr -d '"' || true)" - msha="$(grep -E '^\s+sha1:' "$mir" 2>/dev/null | head -1 | sed -E 's/.*sha1:[[:space:]]*//' | tr -d '"' || true)" - # Prefer the primary_binary sha1 line that is not under installer (second sha1 often PE) + # Prefer the primary_binary sha1 (not installer sha1 under installer:) esha_pe="$(awk '/^primary_binary:/{p=1} p&&/sha1:/{print $2; exit}' "$emb" | tr -d '"' || true)" msha_pe="$(awk '/^primary_binary:/{p=1} p&&/sha1:/{print $2; exit}' "$mir" | tr -d '"' || true)" if [[ -n "$esha_pe" && -n "$msha_pe" && "$esha_pe" != "$msha_pe" ]]; then