From 9ad04de519ccf2e7a908ff29ab2f84ec46b327a3 Mon Sep 17 00:00:00 2001 From: Vyncint Ng Date: Sun, 2 Aug 2026 21:21:20 +0700 Subject: [PATCH] fix: keep the replaced binary executable during update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update stages the new binary in an os.CreateTemp file (created 0600) and copies into it with OpenFile(..., 0o755). But OpenFile ignores the mode for a file that already exists, so the temp — and, after the rename, the installed binary — stayed 0600. The binary was left non-executable and update's follow-up `setup` failed with "fork/exec ...: permission denied", leaving the service on its old inode. Force the mode with an explicit chmod after the copy. Adds a regression test that replaces an existing file and asserts the result stays 0755. Signed-off-by: Vyncint Ng --- CHANGELOG.md | 8 +++++ cmd/openshell-driver-applecontainer/update.go | 7 ++++ .../update_test.go | 36 +++++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e596c1..9635fb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +### Fixed + +- `update` now keeps the replaced binary executable. It staged the new binary in a temp file + created with mode 0600 and relied on `OpenFile` to widen it, but `OpenFile` ignores the mode + argument for a file that already exists — so the installed binary was left non-executable and + the follow-up `setup` failed with "permission denied". The copy now forces the mode + explicitly. (regression in v0.2.4's `update`) + ## [0.2.4] - 2026-08-02 ### Added diff --git a/cmd/openshell-driver-applecontainer/update.go b/cmd/openshell-driver-applecontainer/update.go index 56e7fd7..85ba57b 100644 --- a/cmd/openshell-driver-applecontainer/update.go +++ b/cmd/openshell-driver-applecontainer/update.go @@ -258,6 +258,13 @@ func copyFile(src, dst string, mode os.FileMode) error { _ = out.Close() return err } + // OpenFile's mode is ignored when dst already exists — and it does here: + // replaceBinary copies into an os.CreateTemp file (created 0600). Force + // the mode so the replaced binary keeps its exec bit. + if err := out.Chmod(mode); err != nil { + _ = out.Close() + return err + } return out.Close() } diff --git a/cmd/openshell-driver-applecontainer/update_test.go b/cmd/openshell-driver-applecontainer/update_test.go index 9a85bc1..0299504 100644 --- a/cmd/openshell-driver-applecontainer/update_test.go +++ b/cmd/openshell-driver-applecontainer/update_test.go @@ -81,6 +81,42 @@ func TestExtractBinaryMissing(t *testing.T) { } } +// TestReplaceBinaryKeepsExecBit reproduces the regression where the replaced +// binary landed non-executable: replaceBinary copies into an os.CreateTemp +// file (0600) and OpenFile's mode is ignored for an existing file, so without +// an explicit chmod the renamed target lost its exec bit and `update`'s +// re-setup then failed with "permission denied". +func TestReplaceBinaryKeepsExecBit(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "driver") + if err := os.WriteFile(target, []byte("old"), 0o755); err != nil { + t.Fatal(err) + } + newBin := filepath.Join(dir, "new") + if err := os.WriteFile(newBin, []byte("NEW-BINARY"), 0o755); err != nil { + t.Fatal(err) + } + + if err := replaceBinary(target, newBin); err != nil { + t.Fatal(err) + } + + got, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(got) != "NEW-BINARY" { + t.Errorf("content = %q, want NEW-BINARY", got) + } + info, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0o755 { + t.Errorf("replaced binary mode = %o, want 755 (exec bit must survive)", info.Mode().Perm()) + } +} + func TestVerifyChecksum(t *testing.T) { dir := t.TempDir() archive := filepath.Join(dir, "app.tar.gz")