From 96b4453375cb5121af963c334e6ce38b2bc64093 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Tue, 18 Aug 2026 12:38:37 +0800 Subject: [PATCH 1/7] test: wait for timing conditions instead of racing them Two tests asserted on wall-clock timing that holds on an idle machine and not under the race detector with the whole suite running. The dispatcher test slept 25ms and then required the enqueue-time context to be expired. A deadline is recorded by a timer goroutine, so the clock passes the deadline before anything runs to set the error; it now waits on the context instead. The TUI test gave a key press 100ms to come back. That timeout is only there to prove the prefetch did not run on the event loop, where the press would block until the test releases it, so it is not a latency budget and does not need to be tight. --- pkg/api/ws_dispatcher_test.go | 10 ++++++++-- pkg/ui/tui/searchmedia_test.go | 6 +++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/pkg/api/ws_dispatcher_test.go b/pkg/api/ws_dispatcher_test.go index 60d16ad8c..03be52e97 100644 --- a/pkg/api/ws_dispatcher_test.go +++ b/pkg/api/ws_dispatcher_test.go @@ -585,8 +585,14 @@ func TestWebSocketRunJobStartsMethodTimeoutAtExecution(t *testing.T) { tt.method, )), } - time.Sleep(25 * time.Millisecond) - require.Error(t, enqueuedCtx.Err(), "pre-existing enqueue-time context should be expired") + // Waited for rather than slept past: a deadline is recorded by a timer + // goroutine, so under load the wall clock passes the deadline well + // before anything runs to set the error. + select { + case <-enqueuedCtx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("pre-existing enqueue-time context should be expired") + } beforeRun := time.Now() d.runJob(job) diff --git a/pkg/ui/tui/searchmedia_test.go b/pkg/ui/tui/searchmedia_test.go index 722594f7e..838b006e9 100644 --- a/pkg/ui/tui/searchmedia_test.go +++ b/pkg/ui/tui/searchmedia_test.go @@ -317,9 +317,13 @@ func TestBuildSearchMedia_AutoloadsMoreResults_Integration(t *testing.T) { runner.SimulateArrowDown() close(scrollDone) }() + // If the prefetch ran on the event loop the key press would block until + // releaseNextPage is closed, so this timeout only has to outlast scheduler + // jitter — it is not a latency budget. A short one false-fails when the + // whole suite is running under the race detector. select { case <-scrollDone: - case <-time.After(100 * time.Millisecond): + case <-time.After(5 * time.Second): close(releaseNextPage) <-scrollDone t.Fatal("scrolling blocked while the next page loaded") From b9a268f870da46f6a2a28902675b968b044fa7c1 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Tue, 18 Aug 2026 12:39:01 +0800 Subject: [PATCH 2/7] feat(updater): download and stage a release before installing it Stage is the first half of the rewritten apply pipeline: it picks the archive for this platform out of the signed manifest, downloads it, checks it against the digest the manifest declares, pulls the binary out of it and proves that binary runs. Nothing outside the staging directory is touched, so every failure leaves the device exactly as it was. Nothing calls it yet; install, restart and rollback follow. Selection re-asserts what the manifest claims rather than trusting the detection that led here: the target has to be newer than the running version, at or above the release's min_upgrade_from, not a draft, and match exactly one asset. The version becomes a directory name, so it is asserted to be a single path element. Extraction pulls rather than unpacks. It walks a bounded number of members, ignores everything that is not a regular file, and copies the one member it wants into a path this package chose, so no name out of an archive ever reaches the filesystem. Caps bound the declared archive size, the file kept, and the total inflated bytes the walk may read on the way past. The download bounds silence rather than duration: a legitimate transfer to a MiSTer over a slow link runs for minutes, so a monotonic guard cancels after 90 seconds with no progress instead. Transport timeouts before the first byte land on the same verdict. A write that fails mid-copy is reported as the device's fault, not the release's, because the archive-rejected sentinel is a judgement on the build. The probe is what the no-supervisor platforms depend on: the staged binary has to run and agree about its own version before anything replaces the one that currently works. It catches a wrong architecture, a libc mismatch, a missing shared library, an exec bit a vfat mount dropped, and a noexec mount. The line it matches now comes from config.VersionLine, which the version flag also prints, so the producer and the check cannot drift. --- pkg/cli/cli.go | 6 +- pkg/config/app.go | 22 + pkg/config/app_test.go | 50 + pkg/service/updater/extract.go | 365 ++++++ pkg/service/updater/extract_test.go | 1009 +++++++++++++++ pkg/service/updater/otameta/manifest.go | 12 + pkg/service/updater/otameta/manifest_test.go | 31 + pkg/service/updater/stage.go | 706 +++++++++++ pkg/service/updater/stage_test.go | 1165 ++++++++++++++++++ pkg/service/updater/stall.go | 137 ++ pkg/service/updater/stall_test.go | 182 +++ 11 files changed, 3683 insertions(+), 2 deletions(-) create mode 100644 pkg/config/app_test.go create mode 100644 pkg/service/updater/extract.go create mode 100644 pkg/service/updater/extract_test.go create mode 100644 pkg/service/updater/stage.go create mode 100644 pkg/service/updater/stage_test.go create mode 100644 pkg/service/updater/stall.go create mode 100644 pkg/service/updater/stall_test.go diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go index 1fbff6e78..826b0445d 100644 --- a/pkg/cli/cli.go +++ b/pkg/cli/cli.go @@ -98,7 +98,7 @@ func SetupFlags() *Flags { "send method and params to API and print response", ), Version: flag.Bool( - "version", + config.VersionFlagName, false, "print version and exit", ), @@ -166,7 +166,9 @@ func (f *Flags) Pre(pl platforms.Platform) { flag.Parse() if *f.Version { - _, _ = fmt.Printf("Zaparoo v%s (%s)\n", config.AppVersion, pl.ID()) + // config.VersionLine, not a literal: the self-update probe compares a + // staged binary's output against it, so the two must not drift. + _, _ = fmt.Printf("%s\n", config.VersionLine(config.AppVersion, pl.ID())) os.Exit(0) } } diff --git a/pkg/config/app.go b/pkg/config/app.go index 13a5e53d8..3b164be3d 100644 --- a/pkg/config/app.go +++ b/pkg/config/app.go @@ -20,6 +20,7 @@ package config import ( + "fmt" "strings" "time" ) @@ -56,4 +57,25 @@ const ( CacheDir = "cache" LogUploadURL = "https://logs.zaparoo.org/" MinFreeDiskBytes = 500 * 1024 * 1024 // 500 MB + + // VersionFlagName is the flag that prints VersionLine and exits. The + // self-update probe passes it to a binary it has just downloaded, so the + // name is part of the same frozen contract as the line itself. + VersionFlagName = "version" ) + +// VersionLine is the line the version flag prints, and the line the self-update +// probe looks for in a staged binary's output. +// +// It is a compatibility surface between releases, not a cosmetic string. The +// probe runs in the binary that is already installed and checks what the +// incoming one prints, so it is always the *older* build that decides whether a +// newer release is acceptable. Changing this text would make every device +// already in the field reject the release that changed it, and every release +// after that, with no way to fix it from the new release's side. Both the +// producer and the probe read it from here so they cannot drift, and the probe +// matches this as one line of output rather than the whole stream so that +// adding another line elsewhere stays harmless. +func VersionLine(version, platformID string) string { + return fmt.Sprintf("Zaparoo v%s (%s)", version, platformID) +} diff --git a/pkg/config/app_test.go b/pkg/config/app_test.go new file mode 100644 index 000000000..1befa390b --- /dev/null +++ b/pkg/config/app_test.go @@ -0,0 +1,50 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestVersionLine_IsFrozen is a tripwire, not a restatement of the code. The +// self-update probe runs in the binary a device already has installed and +// compares what a freshly downloaded one prints against this text, so the older +// build is always the one judging the newer. Editing the format would make +// every device in the field refuse the release that changed it and every +// release after it, unrecoverably from the new release's side. +// +// If this test fails, the change is a compatibility break, not a typo fix. +func TestVersionLine_IsFrozen(t *testing.T) { + t.Parallel() + + assert.Equal(t, "Zaparoo v2.11.0 (mister)", VersionLine("2.11.0", "mister")) + assert.Equal(t, "Zaparoo v2.11.0-beta4 (linux)", VersionLine("2.11.0-beta4", "linux")) +} + +// TestVersionFlagName_IsFrozen guards the other half of the same contract: the +// probe invokes a downloaded binary with this flag, so an installed build can +// only ask a future one for its version by the name it knows today. +func TestVersionFlagName_IsFrozen(t *testing.T) { + t.Parallel() + + assert.Equal(t, "version", VersionFlagName) +} diff --git a/pkg/service/updater/extract.go b/pkg/service/updater/extract.go new file mode 100644 index 000000000..17fa9c979 --- /dev/null +++ b/pkg/service/updater/extract.go @@ -0,0 +1,365 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +// Extraction here pulls rather than unpacks. It walks a bounded number of +// archive members, ignores everything that is not a regular file, and copies +// the one member it wants into a path this package chose. No name out of the +// archive ever reaches the filesystem, which is what removes zip-slip, "..", +// absolute paths and duplicate names as things to get right: there is no code +// path where an archive decides where a byte lands. + +package updater + +import ( + "archive/tar" + "archive/zip" + "compress/gzip" + "context" + "crypto/sha256" + "crypto/subtle" + "encoding/hex" + "errors" + "fmt" + "io" + "os" + "regexp" + "strings" +) + +const ( + archiveExtTarGz = ".tar.gz" + archiveExtZip = ".zip" + + // semverPattern matches a version inside an archive member name. + semverPattern = `(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)` + + `(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?` + + `(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?` +) + +// ctxReader ends a read once the context is done. The archive walks need it +// because they are otherwise uninterruptible: a gzip stream cannot seek, so tar +// has to inflate every member it skips on the way past, and a cancelled staging +// attempt would keep decompressing tens of megabytes on a device that is trying +// to shut down. +type ctxReader struct { + ctx context.Context + source io.Reader +} + +func (r *ctxReader) Read(p []byte) (int, error) { + if err := r.ctx.Err(); err != nil { + return 0, fmt.Errorf("reading the update archive: %w", err) + } + //nolint:wrapcheck // a reader wrapper has to pass io.EOF and the source's errors through unchanged + return r.source.Read(p) +} + +// stagedSink is where a member copied out of an archive lands. *os.File is the +// only implementation outside tests; the interface is here so a destination that +// fails mid-write can be exercised without filling a real disk. +type stagedSink interface { + io.Writer + Sync() error + Close() error +} + +// errWriter remembers the destination's own failures. io.Copy fuses the two +// directions into a single error value, and here they mean opposite things: a +// read that fails is the archive's problem, a write that fails is the device's. +type errWriter struct { + dest io.Writer + err error +} + +func (w *errWriter) Write(p []byte) (int, error) { + n, err := w.dest.Write(p) + if err != nil { + w.err = err + } + //nolint:wrapcheck // a writer wrapper has to pass the destination's error through unchanged + return n, err +} + +// extractBinary opens the archive once, proves the bytes sitting on disk are +// still the ones the signed manifest describes, and copies the executable out of +// that same open file. +func (s *stager) extractBinary(ctx context.Context, archivePath, ext string, want []byte, destPath string) error { + //nolint:gosec // the path is built by this package inside its own staging directory + f, err := os.Open(archivePath) + if err != nil { + return fmt.Errorf("opening the update archive: %w", err) + } + defer closeQuietly(f, "update archive") + + size, err := verifyOpenArchive(ctx, f, want) + if err != nil { + return err + } + // Only the tar path reads sequentially; zip addresses the handle directly and + // does not care where the offset is. Rewinding both keeps that an + // implementation detail of the format rather than of this function. + if _, err := f.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("rewinding the update archive: %w", err) + } + + switch ext { + case archiveExtTarGz: + return s.extractFromTarGz(ctx, f, destPath) + case archiveExtZip: + return s.extractFromZip(ctx, f, size, destPath) + default: + return fmt.Errorf("%w: %q is not an archive type this build unpacks", ErrArchiveRejected, ext) + } +} + +// verifyOpenArchive re-checks the archive against the manifest digest, reading +// through the handle extraction is about to use. +// +// The download already hashed these bytes on their way to disk. This hashes them +// on the way back off it, which is a different claim: in between there is a +// close, and the storage these devices run on has been observed acknowledging an +// fsync and later returning zeroed pages, so "the bytes that arrived were right" +// does not establish "the bytes about to be installed are right". Reading +// through the same open file rather than re-opening by path is what ties the two +// reads to one inode. +// +// Without this the .tar.gz platforms have no integrity check on the second read +// at all: tar carries no member checksum, and the walk stops at the +// end-of-archive marker without ever driving the gzip stream to its trailer. The +// .zip platforms get per-member CRC32 for free, which is weaker than this and +// covers only the member that is read. +func verifyOpenArchive(ctx context.Context, f *os.File, want []byte) (int64, error) { + digest := sha256.New() + size, err := io.Copy(digest, &ctxReader{ctx: ctx, source: f}) + if err != nil { + return 0, fmt.Errorf("re-reading the update archive: %w", err) + } + + got := digest.Sum(nil) + if subtle.ConstantTimeCompare(got, want) != 1 { + return 0, fmt.Errorf("%w: the archive on disk hashes to %s, the manifest declares %s", + ErrChecksumMismatch, hex.EncodeToString(got), hex.EncodeToString(want)) + } + return size, nil +} + +func (s *stager) extractFromTarGz(ctx context.Context, f *os.File, destPath string) error { + gz, err := gzip.NewReader(&ctxReader{ctx: ctx, source: f}) + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return fmt.Errorf("reading the update archive was cancelled: %w", ctxErr) + } + return fmt.Errorf("%w: reading the update archive: %w", ErrArchiveRejected, err) + } + defer closeQuietly(gz, "update archive decompressor") + + // The member count bounds how many entries the walk visits; this bounds how + // much content getting to them can cost. A skipped member still has to be + // inflated in full, because gzip cannot seek past one, so without a ceiling + // here a bomb ahead of the binary would run unbounded. + inflated := &io.LimitedReader{R: gz, N: s.maxInflatedBytes + 1} + overBudget := func() bool { return inflated.N <= 0 } + + tr := tar.NewReader(inflated) + found := false + members := 0 + for { + header, nextErr := tr.Next() + if errors.Is(nextErr, io.EOF) { + break + } + if nextErr != nil { + // Caller intent first. ErrArchiveRejected is a permanent verdict on the + // release, so returning it for what was really a shutdown would condemn + // a build nothing had actually found fault with. + if ctxErr := ctx.Err(); ctxErr != nil { + return fmt.Errorf("reading the update archive was cancelled: %w", ctxErr) + } + if overBudget() { + return fmt.Errorf("%w: more than %d bytes of content in it", + ErrArchiveRejected, s.maxInflatedBytes) + } + return fmt.Errorf("%w: reading the update archive: %w", ErrArchiveRejected, nextErr) + } + + members++ + if members > maxArchiveMembers { + return fmt.Errorf("%w: more than %d members", ErrArchiveRejected, maxArchiveMembers) + } + + // Regular files only. A symlink, hardlink, device node, fifo or + // directory entry is skipped rather than reasoned about. + if header.Typeflag != tar.TypeReg || !s.wantsMember(header.Name) { + continue + } + if found { + return fmt.Errorf("%w: more than one %s", ErrArchiveRejected, s.binaryName) + } + if header.Size > s.maxFileBytes { + return fmt.Errorf("%w: %s declares %d bytes, over the %d byte limit", + ErrArchiveRejected, header.Name, header.Size, s.maxFileBytes) + } + if copyErr := copyStagedFile(ctx, tr, destPath, s.maxFileBytes); copyErr != nil { + // copyStagedFile already reports a cancellation as one, so only the + // budget needs separating out here. + if ctx.Err() == nil && overBudget() { + return fmt.Errorf("%w: more than %d bytes of content in it", + ErrArchiveRejected, s.maxInflatedBytes) + } + return copyErr + } + found = true + } + + if !found { + return fmt.Errorf("%w: no %s in it", ErrArchiveRejected, s.binaryName) + } + return nil +} + +func (s *stager) extractFromZip(ctx context.Context, f *os.File, size int64, destPath string) error { + // No total-content ceiling here, unlike the tar walk: a zip member that is + // not wanted is never opened, so nothing but the binary is ever inflated and + // maxFileBytes already bounds that. + r, err := zip.NewReader(f, size) + if err != nil { + return fmt.Errorf("%w: reading the update archive: %w", ErrArchiveRejected, err) + } + + if len(r.File) > maxArchiveMembers { + return fmt.Errorf("%w: more than %d members", ErrArchiveRejected, maxArchiveMembers) + } + + found := false + for _, member := range r.File { + if ctxErr := ctx.Err(); ctxErr != nil { + return fmt.Errorf("reading the update archive: %w", ctxErr) + } + // Regular files only, same as the tar walk: the mode bits are what + // carry a zip symlink, and a directory entry is not a file. + if !member.Mode().IsRegular() || !s.wantsMember(member.Name) { + continue + } + if found { + return fmt.Errorf("%w: more than one %s", ErrArchiveRejected, s.binaryName) + } + if declared := member.FileInfo().Size(); declared > s.maxFileBytes { + return fmt.Errorf("%w: %s declares %d bytes, over the %d byte limit", + ErrArchiveRejected, member.Name, declared, s.maxFileBytes) + } + + rc, openErr := member.Open() + if openErr != nil { + return fmt.Errorf("%w: reading %s: %w", ErrArchiveRejected, member.Name, openErr) + } + copyErr := copyStagedFile(ctx, rc, destPath, s.maxFileBytes) + closeQuietly(rc, "update archive member") + if copyErr != nil { + return copyErr + } + found = true + } + + if !found { + return fmt.Errorf("%w: no %s in it", ErrArchiveRejected, s.binaryName) + } + return nil +} + +// wantsMember reports whether a member is the binary being staged. It has to be +// the member's whole name: release archives are flat, so a nested path claiming +// to be the binary is not something a genuine build produces. +func (s *stager) wantsMember(name string) bool { + if strings.ContainsAny(name, `/\`) { + return false + } + return matchExecutableName(s.binaryName, s.goos, s.goarch, name) +} + +// matchExecutableName reports whether an archive member names the executable +// cmd: the bare name, optionally carrying a version and an os/arch pair, with +// an optional .exe. It reimplements the rule go-selfupdate applies inside its +// own decompressor, because nothing here goes through that decompressor and the +// rule decides which file gets installed. +func matchExecutableName(cmd, goos, goarch, target string) bool { + base := strings.TrimSuffix(cmd, ".exe") + if base == "" { + return false + } + // Every part is quoted, so a name with punctuation in it stays a name. The + // MiSTer builds are called zaparoo.sh, which without quoting would match + // anything in that position. + pattern := regexp.MustCompile(fmt.Sprintf( + `^%s([_-]v?%s)?([_-]%s[_-]%s)?(\.exe)?$`, + regexp.QuoteMeta(base), + semverPattern, + regexp.QuoteMeta(goos), + regexp.QuoteMeta(goarch), + )) + return pattern.MatchString(target) +} + +// copyStagedFile writes an archive member to a path this package chose. The +// limit is enforced against the bytes that actually arrive rather than the size +// the archive declares, so a header that lies about its size is caught mid-copy +// instead of being trusted to fill a disk. +func copyStagedFile(ctx context.Context, src io.Reader, destPath string, limit int64) error { + //nolint:gosec // the path is built by this package inside its own staging directory + f, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, stagedFilePerm) + if err != nil { + return fmt.Errorf("creating the staged update binary: %w", err) + } + return copyIntoSink(ctx, src, f, limit) +} + +// copyIntoSink is copyStagedFile with the destination already open. It owns +// flushing and closing it, whichever way the copy goes. +func copyIntoSink(ctx context.Context, src io.Reader, dest stagedSink, limit int64) error { + sink := &errWriter{dest: dest} + limited := io.LimitReader(&ctxReader{ctx: ctx, source: src}, limit+1) + written, copyErr := io.Copy(sink, limited) + syncErr := dest.Sync() + closeErr := dest.Close() + + switch { + case copyErr != nil && ctx.Err() != nil: + return fmt.Errorf("extracting the update binary was cancelled: %w", ctx.Err()) + case copyErr != nil && sink.err != nil: + // The device failed, not the release. ErrArchiveRejected is a verdict on + // the build itself, so a full or failing SD card must not earn one: the + // flush below already reports the same physical fault as an ordinary + // error, and so does the download's own write path. + return fmt.Errorf("writing the staged update binary: %w", sink.err) + case copyErr != nil: + return fmt.Errorf("%w: extracting the update binary: %w", ErrArchiveRejected, copyErr) + case syncErr != nil: + return fmt.Errorf("flushing the staged update binary to disk: %w", syncErr) + case closeErr != nil: + return fmt.Errorf("closing the staged update binary: %w", closeErr) + } + + if written > limit { + return fmt.Errorf("%w: the update binary is larger than the %d byte limit", + ErrArchiveRejected, limit) + } + if written == 0 { + return fmt.Errorf("%w: the update binary is empty", ErrArchiveRejected) + } + return nil +} diff --git a/pkg/service/updater/extract_test.go b/pkg/service/updater/extract_test.go new file mode 100644 index 000000000..2128e8d9e --- /dev/null +++ b/pkg/service/updater/extract_test.go @@ -0,0 +1,1009 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +package updater + +import ( + "archive/tar" + "archive/zip" + "bytes" + "compress/gzip" + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// tarMember describes one entry to write into a test tar. The size in the +// header always matches the body, because tar.Writer refuses to let a header +// lie about it; the guard against an oversized declared size is exercised by +// lowering the stager's limit instead. +type tarMember struct { + name string + linkname string + body []byte + mode int64 + typeflag byte +} + +// zipMember describes one entry to write into a test zip. The mode carries what +// a zip uses instead of a type flag: a symlink or a directory is a mode bit. +type zipMember struct { + name string + body []byte + mode fs.FileMode +} + +func writeTarGz(t *testing.T, path string, members []tarMember) { + t.Helper() + + var buf bytes.Buffer + gz, err := gzip.NewWriterLevel(&buf, gzip.BestSpeed) + require.NoError(t, err) + tw := tar.NewWriter(gz) + + for _, m := range members { + header := &tar.Header{ + Name: m.name, + Linkname: m.linkname, + Typeflag: m.typeflag, + Mode: m.mode, + } + if header.Typeflag == 0 { + header.Typeflag = tar.TypeReg + } + if header.Mode == 0 { + header.Mode = 0o644 + } + if header.Typeflag == tar.TypeReg { + header.Size = int64(len(m.body)) + } + require.NoError(t, tw.WriteHeader(header)) + if header.Typeflag == tar.TypeReg && len(m.body) > 0 { + _, writeErr := tw.Write(m.body) + require.NoError(t, writeErr) + } + } + + require.NoError(t, tw.Close()) + require.NoError(t, gz.Close()) + require.NoError(t, os.WriteFile(path, buf.Bytes(), 0o600)) +} + +func writeZip(t *testing.T, path string, members []zipMember) { + t.Helper() + + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + + for _, m := range members { + header := &zip.FileHeader{Name: m.name, Method: zip.Deflate} + mode := m.mode + if mode == 0 { + mode = 0o644 + } + header.SetMode(mode) + w, err := zw.CreateHeader(header) + require.NoError(t, err) + if len(m.body) > 0 { + _, writeErr := w.Write(m.body) + require.NoError(t, writeErr) + } + } + + require.NoError(t, zw.Close()) + require.NoError(t, os.WriteFile(path, buf.Bytes(), 0o600)) +} + +// extractHarness lays a tree out so an escape is visible: the archive, the +// payload directory extraction is allowed to write into, and a sibling holding +// a file that must never change. +type extractHarness struct { + stager *stager + base string + archivePath string + destPath string + payloadDir string +} + +func newExtractHarness(t *testing.T, ext string) *extractHarness { + t.Helper() + + base := t.TempDir() + payloadDir := filepath.Join(base, "staging", payloadSubdir) + require.NoError(t, os.MkdirAll(payloadDir, 0o750)) + require.NoError(t, os.MkdirAll(filepath.Join(base, "archive"), 0o750)) + require.NoError(t, os.MkdirAll(filepath.Join(base, "outside"), 0o750)) + require.NoError(t, os.WriteFile( + filepath.Join(base, "outside", "sentinel"), []byte("untouched"), 0o600)) + + return &extractHarness{ + stager: &stager{ + binaryName: "zaparoo", + goos: "linux", + goarch: "amd64", + maxFileBytes: maxStagedFileBytes, + maxInflatedBytes: maxArchiveInflatedBytes, + }, + base: base, + archivePath: filepath.Join(base, "archive", "release"+ext), + destPath: filepath.Join(payloadDir, "zaparoo"), + payloadDir: payloadDir, + } +} + +// extract runs extraction and asserts the only thing it changed is inside the +// payload directory, which is the whole point of naming the destination +// ourselves rather than unpacking archive-supplied paths. +func (h *extractHarness) extract(t *testing.T, ext string) error { + t.Helper() + + // Hashed as the archive stands right now, so a test that deliberately + // corrupts one still exercises the format handling instead of stopping at the + // on-disk digest check. TestExtractBinary_RejectsBytesChangedOnDisk covers + // that check on its own. + return h.extractWithDigest(t, ext, archiveDigest(t, h.archivePath)) +} + +func (h *extractHarness) extractWithDigest(t *testing.T, ext string, want []byte) error { + t.Helper() + + before := snapshotOutside(t, h.base, h.payloadDir) + err := h.stager.extractBinary(context.Background(), h.archivePath, ext, want, h.destPath) + assert.Equal(t, before, snapshotOutside(t, h.base, h.payloadDir), + "extraction wrote outside the payload directory") + return err +} + +func archiveDigest(t *testing.T, path string) []byte { + t.Helper() + + body, err := os.ReadFile(path) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + sum := sha256.Sum256(body) + return sum[:] +} + +// snapshotOutside records the whole tree except the payload directory. +func snapshotOutside(t *testing.T, base, payloadDir string) map[string]string { + t.Helper() + + found := make(map[string]string) + err := filepath.WalkDir(base, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if path == payloadDir { + return fs.SkipDir + } + rel, relErr := filepath.Rel(base, path) + if relErr != nil { + return fmt.Errorf("relative path of %s: %w", path, relErr) + } + if d.IsDir() { + found[rel] = "dir" + return nil + } + body, readErr := os.ReadFile(path) //nolint:gosec // test tree under t.TempDir + if readErr != nil { + return fmt.Errorf("reading %s: %w", path, readErr) + } + sum := sha256.Sum256(body) + found[rel] = hex.EncodeToString(sum[:]) + return nil + }) + require.NoError(t, err) + return found +} + +func TestMatchExecutableName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cmd string + goos string + goarch string + target string + want bool + }{ + {name: "bare name", cmd: "zaparoo", goos: "linux", goarch: "amd64", target: "zaparoo", want: true}, + { + name: "version with underscore", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo_2.11.0", want: true, + }, + { + name: "version with dash", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo-2.11.0", want: true, + }, + { + name: "version with v prefix", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo_v2.11.0", want: true, + }, + { + name: "prerelease version", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo-2.11.0-beta.1", want: true, + }, + { + name: "os and arch", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo_linux_amd64", want: true, + }, + { + name: "version and os and arch", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo_2.11.0_linux_amd64", want: true, + }, + { + name: "exe suffix accepted", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo.exe", want: true, + }, + { + name: "windows binary", cmd: "Zaparoo.exe", goos: "windows", goarch: "amd64", + target: "Zaparoo.exe", want: true, + }, + { + name: "windows binary without extension", cmd: "Zaparoo.exe", goos: "windows", goarch: "amd64", + target: "Zaparoo", want: true, + }, + { + name: "match is case sensitive", cmd: "Zaparoo.exe", goos: "windows", goarch: "amd64", + target: "zaparoo.exe", want: false, + }, + { + name: "mister binary keeps its sh name", cmd: "zaparoo.sh", goos: "linux", goarch: "arm", + target: "zaparoo.sh", want: true, + }, + { + name: "mister binary with version", cmd: "zaparoo.sh", goos: "linux", goarch: "arm", + target: "zaparoo.sh_2.11.0", want: true, + }, + // The dot in zaparoo.sh has to stay a dot. Without QuoteMeta it would + // match any character in that position. + { + name: "dot in the name is literal", cmd: "zaparoo.sh", goos: "linux", goarch: "arm", + target: "zaparoosh", want: false, + }, + { + name: "mister binary is not the plain name", cmd: "zaparoo.sh", goos: "linux", goarch: "arm", + target: "zaparoo", want: false, + }, + { + name: "sh suffix is not the plain binary", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo.sh", want: false, + }, + { + name: "other os", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo_windows_amd64", want: false, + }, + { + name: "other arch", cmd: "zaparoo", goos: "linux", goarch: "amd64", + target: "zaparoo_linux_arm64", want: false, + }, + // arm must not match an arm64 archive member. + { + name: "arm does not match arm64", cmd: "zaparoo", goos: "linux", goarch: "arm", + target: "zaparoo_linux_arm64", want: false, + }, + {name: "licence", cmd: "zaparoo", goos: "linux", goarch: "amd64", target: "LICENSE.txt", want: false}, + {name: "trailing junk", cmd: "zaparoo", goos: "linux", goarch: "amd64", target: "zaparoo2", want: false}, + {name: "nested path", cmd: "zaparoo", goos: "linux", goarch: "amd64", target: "bin/zaparoo", want: false}, + {name: "empty target", cmd: "zaparoo", goos: "linux", goarch: "amd64", target: "", want: false}, + {name: "empty command", cmd: "", goos: "linux", goarch: "amd64", target: "zaparoo", want: false}, + {name: "command is only an extension", cmd: ".exe", goos: "windows", goarch: "amd64", target: "", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := matchExecutableName(tt.cmd, tt.goos, tt.goarch, tt.target) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestWantsMember_RejectsAnythingWithAPathInIt(t *testing.T) { + t.Parallel() + + s := &stager{binaryName: "zaparoo", goos: "linux", goarch: "amd64"} + + assert.True(t, s.wantsMember("zaparoo")) + assert.False(t, s.wantsMember("bin/zaparoo")) + assert.False(t, s.wantsMember(`bin\zaparoo`)) + assert.False(t, s.wantsMember("../zaparoo")) + assert.False(t, s.wantsMember("/zaparoo")) + assert.False(t, s.wantsMember("./zaparoo")) +} + +func TestExtractFromTarGz_TakesOnlyTheBinary(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtTarGz) + binary := []byte("this stands in for the executable") + writeTarGz(t, h.archivePath, []tarMember{ + {name: "LICENSE.txt", body: []byte("gpl")}, + {name: "README.txt", body: []byte("readme")}, + {name: "zaparoo", body: binary, mode: 0o755}, + }) + + require.NoError(t, h.extract(t, archiveExtTarGz)) + + got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + assert.Equal(t, binary, got) + + // The licence and readme are in the archive and are deliberately not + // written: extraction pulls what it wants, it does not unpack. + entries, err := os.ReadDir(h.payloadDir) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, "zaparoo", entries[0].Name()) +} + +func TestExtractFromZip_TakesOnlyTheBinary(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtZip) + binary := []byte("this stands in for the executable") + writeZip(t, h.archivePath, []zipMember{ + {name: "LICENSE.txt", body: []byte("gpl")}, + {name: "README.txt", body: []byte("readme")}, + {name: "zaparoo", body: binary, mode: 0o755}, + }) + + require.NoError(t, h.extract(t, archiveExtZip)) + + got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + assert.Equal(t, binary, got) + + entries, err := os.ReadDir(h.payloadDir) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, "zaparoo", entries[0].Name()) +} + +func TestExtractFromTarGz_VersionedMemberName(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtTarGz) + binary := []byte("versioned") + writeTarGz(t, h.archivePath, []tarMember{ + {name: "zaparoo_2.11.0_linux_amd64", body: binary, mode: 0o755}, + }) + + require.NoError(t, h.extract(t, archiveExtTarGz)) + + // It landed under the name this package chose, not the one in the archive. + got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + assert.Equal(t, binary, got) +} + +func TestExtractBinary_UnknownExtension(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, ".7z") + require.NoError(t, os.WriteFile(h.archivePath, []byte("not an archive"), 0o600)) + + err := h.extract(t, ".7z") + require.ErrorIs(t, err, ErrArchiveRejected) + assert.NoFileExists(t, h.destPath) +} + +// TestExtractBinary_RejectsBytesChangedOnDisk is the claim the download's own +// hash cannot make. That one proves the bytes that arrived were right; this one +// proves the bytes about to be installed are. Between the two there is a close, +// and the storage these devices run on has been seen acknowledging an fsync and +// later handing back zeroed pages, so the archive is re-read through the handle +// extraction is about to use rather than trusted. +func TestExtractBinary_RejectsBytesChangedOnDisk(t *testing.T) { + t.Parallel() + + for _, ext := range []string{archiveExtTarGz, archiveExtZip} { + t.Run(ext, func(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, ext) + writeArchive(t, h.archivePath, ext, "zaparoo", []byte("the binary")) + + // What the download verified, before the disk changed under it. + want := archiveDigest(t, h.archivePath) + + body, err := os.ReadFile(h.archivePath) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + body[len(body)/2] ^= 0xff + //nolint:gosec // G703: test path under t.TempDir + require.NoError(t, os.WriteFile(h.archivePath, body, 0o600)) + + err = h.extractWithDigest(t, ext, want) + require.ErrorIs(t, err, ErrChecksumMismatch) + assert.NoFileExists(t, h.destPath, "a binary was staged out of an archive that no longer verifies") + }) + } +} + +// TestExtractBinary_StopsOnCancellation asserts a cancelled staging attempt gets +// no further than the digest re-read, and is not blamed on the archive. +func TestExtractBinary_StopsOnCancellation(t *testing.T) { + t.Parallel() + + for _, ext := range []string{archiveExtTarGz, archiveExtZip} { + t.Run(ext, func(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, ext) + writeArchive(t, h.archivePath, ext, "zaparoo", []byte("the binary")) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := h.stager.extractBinary(ctx, h.archivePath, ext, archiveDigest(t, h.archivePath), h.destPath) + require.ErrorIs(t, err, context.Canceled) + // Nothing was wrong with the archive, so nothing may say there was. + require.NotErrorIs(t, err, ErrChecksumMismatch) + require.NotErrorIs(t, err, ErrArchiveRejected) + assert.NoFileExists(t, h.destPath) + }) + } +} + +// TestExtractWalk_StopsOnCancellation covers the walk itself, which is the +// stretch that is otherwise uninterruptible: a gzip stream cannot be seeked past, +// so tar inflates every member it skips, and a shutdown part-way through would +// keep decompressing tens of megabytes on a device trying to stop. The walks are +// called directly because reaching them past the digest read needs a context that +// is live for one and dead for the other. +func TestExtractWalk_StopsOnCancellation(t *testing.T) { + t.Parallel() + + tests := []struct { + walk func(*extractHarness, context.Context, *os.File, int64) error + ext string + }{ + { + ext: archiveExtTarGz, + walk: func(h *extractHarness, ctx context.Context, f *os.File, _ int64) error { + return h.stager.extractFromTarGz(ctx, f, h.destPath) + }, + }, + { + ext: archiveExtZip, + walk: func(h *extractHarness, ctx context.Context, f *os.File, size int64) error { + return h.stager.extractFromZip(ctx, f, size, h.destPath) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.ext, func(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, tt.ext) + writeArchive(t, h.archivePath, tt.ext, "zaparoo", []byte("the binary")) + + f, err := os.Open(h.archivePath) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + t.Cleanup(func() { _ = f.Close() }) + info, err := f.Stat() + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err = tt.walk(h, ctx, f, info.Size()) + require.ErrorIs(t, err, context.Canceled) + // ErrArchiveRejected is a permanent verdict on the release. A walk that + // was interrupted reached no verdict at all, so it must not return one. + require.NotErrorIs(t, err, ErrArchiveRejected) + assert.NoFileExists(t, h.destPath) + }) + } +} + +// TestExtractFromTarGz_InflateBudget bounds what reaching the binary can be made +// to cost. The member cap alone does not: a skipped member still has to be +// inflated in full, so content ahead of the binary is work the walk cannot +// decline. +func TestExtractFromTarGz_InflateBudget(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtTarGz) + // Compresses to almost nothing on the wire and to well over the budget once + // inflated, which is the shape of the attack. + writeTarGz(t, h.archivePath, []tarMember{ + {name: "filler.bin", body: make([]byte, 64<<10)}, + {name: "zaparoo", body: []byte("the binary"), mode: 0o755}, + }) + h.stager.maxInflatedBytes = 4 << 10 + + err := h.extract(t, archiveExtTarGz) + require.ErrorIs(t, err, ErrArchiveRejected) + assert.Contains(t, err.Error(), "bytes of content in it") + assert.NoFileExists(t, h.destPath) +} + +// TestExtractFromZip_NeedsNoInflateBudget is the other half of that: a zip member +// the walk does not want is never opened, so the same filler costs nothing and +// must not be refused. +func TestExtractFromZip_NeedsNoInflateBudget(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtZip) + binary := []byte("the binary") + writeZip(t, h.archivePath, []zipMember{ + {name: "filler.bin", body: make([]byte, 64<<10)}, + {name: "zaparoo", body: binary, mode: 0o755}, + }) + h.stager.maxInflatedBytes = 4 << 10 + + require.NoError(t, h.extract(t, archiveExtZip)) + + got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + assert.Equal(t, binary, got) +} + +// writeArchive writes a minimal release archive in either format. +func writeArchive(t *testing.T, path, ext, memberName string, binary []byte) { + t.Helper() + + switch ext { + case archiveExtTarGz: + writeTarGz(t, path, []tarMember{{name: memberName, body: binary, mode: 0o755}}) + case archiveExtZip: + writeZip(t, path, []zipMember{{name: memberName, body: binary, mode: 0o755}}) + default: + t.Fatalf("unsupported archive extension %q", ext) + } +} + +func TestExtractFromTarGz_HostileMembers(t *testing.T) { + t.Parallel() + + binary := []byte("the binary") + + crowded := make([]tarMember, 0, maxArchiveMembers+1) + for i := range maxArchiveMembers { + crowded = append(crowded, tarMember{name: fmt.Sprintf("filler-%d", i), body: []byte("x")}) + } + crowded = append(crowded, tarMember{name: "zaparoo", body: binary}) + + tests := []struct { + name string + wantMsg string + members []tarMember + limit int64 + }{ + { + name: "traversal name", + members: []tarMember{ + {name: "../../etc/passwd", body: []byte("root:x:0:0")}, + {name: "LICENSE.txt", body: []byte("gpl")}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "absolute name", + members: []tarMember{ + {name: "/etc/passwd", body: []byte("root:x:0:0")}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "nested binary", + members: []tarMember{ + {name: "bin/zaparoo", body: binary}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "symlink standing in for the binary", + members: []tarMember{ + {name: "zaparoo", linkname: "/etc/passwd", typeflag: tar.TypeSymlink}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "hardlink standing in for the binary", + members: []tarMember{ + {name: "zaparoo", linkname: "LICENSE.txt", typeflag: tar.TypeLink}, + {name: "LICENSE.txt", body: []byte("gpl")}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "character device", + members: []tarMember{ + {name: "zaparoo", typeflag: tar.TypeChar}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "block device", + members: []tarMember{ + {name: "zaparoo", typeflag: tar.TypeBlock}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "fifo", + members: []tarMember{ + {name: "zaparoo", typeflag: tar.TypeFifo}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "directory using the binary name", + members: []tarMember{ + {name: "zaparoo", typeflag: tar.TypeDir, mode: 0o755}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "no binary at all", + members: []tarMember{ + {name: "LICENSE.txt", body: []byte("gpl")}, + {name: "README.txt", body: []byte("readme")}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "two different names both match", + members: []tarMember{ + {name: "zaparoo", body: binary}, + {name: "zaparoo_linux_amd64", body: []byte("a different binary")}, + }, + wantMsg: "more than one zaparoo", + }, + { + name: "duplicate names", + members: []tarMember{ + {name: "zaparoo", body: binary}, + {name: "zaparoo", body: []byte("a different binary")}, + }, + wantMsg: "more than one zaparoo", + }, + { + name: "declared size over the limit", + members: []tarMember{ + {name: "zaparoo", body: bytes.Repeat([]byte("x"), 128)}, + }, + limit: 16, + wantMsg: "over the 16 byte limit", + }, + { + name: "too many members", + members: crowded, + wantMsg: fmt.Sprintf("more than %d members", maxArchiveMembers), + }, + { + name: "empty binary", + members: []tarMember{ + {name: "zaparoo", body: nil}, + }, + wantMsg: "the update binary is empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtTarGz) + if tt.limit > 0 { + h.stager.maxFileBytes = tt.limit + } + writeTarGz(t, h.archivePath, tt.members) + + err := h.extract(t, archiveExtTarGz) + require.ErrorIs(t, err, ErrArchiveRejected) + assert.Contains(t, err.Error(), tt.wantMsg) + }) + } +} + +func TestExtractFromZip_HostileMembers(t *testing.T) { + t.Parallel() + + binary := []byte("the binary") + + crowded := make([]zipMember, 0, maxArchiveMembers+1) + for i := range maxArchiveMembers { + crowded = append(crowded, zipMember{name: fmt.Sprintf("filler-%d", i), body: []byte("x")}) + } + crowded = append(crowded, zipMember{name: "zaparoo", body: binary}) + + tests := []struct { + name string + wantMsg string + members []zipMember + limit int64 + }{ + { + name: "traversal name", + members: []zipMember{ + {name: "../../etc/passwd", body: []byte("root:x:0:0")}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "windows traversal name", + members: []zipMember{ + {name: `..\zaparoo`, body: binary}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "absolute name", + members: []zipMember{ + {name: "/etc/passwd", body: []byte("root:x:0:0")}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "nested binary", + members: []zipMember{ + {name: "bin/zaparoo", body: binary}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "symlink standing in for the binary", + members: []zipMember{ + {name: "zaparoo", body: []byte("/etc/passwd"), mode: fs.ModeSymlink | 0o777}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "directory using the binary name", + members: []zipMember{ + {name: "zaparoo", mode: fs.ModeDir | 0o755}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "no binary at all", + members: []zipMember{ + {name: "LICENSE.txt", body: []byte("gpl")}, + }, + wantMsg: "no zaparoo in it", + }, + { + name: "two different names both match", + members: []zipMember{ + {name: "zaparoo", body: binary}, + {name: "zaparoo_linux_amd64", body: []byte("a different binary")}, + }, + wantMsg: "more than one zaparoo", + }, + { + name: "duplicate names", + members: []zipMember{ + {name: "zaparoo", body: binary}, + {name: "zaparoo", body: []byte("a different binary")}, + }, + wantMsg: "more than one zaparoo", + }, + { + name: "declared size over the limit", + members: []zipMember{ + {name: "zaparoo", body: bytes.Repeat([]byte("x"), 128)}, + }, + limit: 16, + wantMsg: "over the 16 byte limit", + }, + { + name: "too many members", + members: crowded, + wantMsg: fmt.Sprintf("more than %d members", maxArchiveMembers), + }, + { + name: "empty binary", + members: []zipMember{ + {name: "zaparoo", body: nil}, + }, + wantMsg: "the update binary is empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtZip) + if tt.limit > 0 { + h.stager.maxFileBytes = tt.limit + } + writeZip(t, h.archivePath, tt.members) + + err := h.extract(t, archiveExtZip) + require.ErrorIs(t, err, ErrArchiveRejected) + assert.Contains(t, err.Error(), tt.wantMsg) + }) + } +} + +func TestExtractFromTarGz_CorruptArchive(t *testing.T) { + t.Parallel() + + t.Run("not gzip at all", func(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtTarGz) + require.NoError(t, os.WriteFile(h.archivePath, []byte("this is not gzip"), 0o600)) + + err := h.extract(t, archiveExtTarGz) + require.ErrorIs(t, err, ErrArchiveRejected) + assert.NoFileExists(t, h.destPath) + }) + + t.Run("truncated tar inside valid gzip", func(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtTarGz) + full := filepath.Join(t.TempDir(), "full.tar.gz") + writeTarGz(t, full, []tarMember{ + {name: "LICENSE.txt", body: bytes.Repeat([]byte("gpl"), 4096)}, + {name: "zaparoo", body: bytes.Repeat([]byte("bin"), 4096)}, + }) + body, err := os.ReadFile(full) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + //nolint:gosec // G703: test path under t.TempDir + require.NoError(t, os.WriteFile(h.archivePath, body[:len(body)/2], 0o600)) + + err = h.extract(t, archiveExtTarGz) + require.ErrorIs(t, err, ErrArchiveRejected) + }) +} + +func TestExtractFromZip_CorruptArchive(t *testing.T) { + t.Parallel() + + h := newExtractHarness(t, archiveExtZip) + require.NoError(t, os.WriteFile(h.archivePath, []byte("this is not a zip"), 0o600)) + + err := h.extract(t, archiveExtZip) + require.ErrorIs(t, err, ErrArchiveRejected) + assert.NoFileExists(t, h.destPath) +} + +// TestCopyStagedFile_LimitsBytesThatArrive covers the backstop no archive header +// can talk its way past: the limit is applied to what actually turns up, so a +// member that streams more than it declared is still caught. +func TestCopyStagedFile_LimitsBytesThatArrive(t *testing.T) { + t.Parallel() + + dest := filepath.Join(t.TempDir(), "zaparoo") + err := copyStagedFile(context.Background(), strings.NewReader(strings.Repeat("x", 64)), dest, 16) + + require.ErrorIs(t, err, ErrArchiveRejected) + assert.Contains(t, err.Error(), "larger than the 16 byte limit") +} + +func TestCopyStagedFile_ExactLimitIsAccepted(t *testing.T) { + t.Parallel() + + dest := filepath.Join(t.TempDir(), "zaparoo") + require.NoError(t, copyStagedFile(context.Background(), strings.NewReader("0123456789abcdef"), dest, 16)) + + got, err := os.ReadFile(dest) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + assert.Equal(t, "0123456789abcdef", string(got)) +} + +func TestCopyStagedFile_RefusesToOverwrite(t *testing.T) { + t.Parallel() + + dest := filepath.Join(t.TempDir(), "zaparoo") + require.NoError(t, os.WriteFile(dest, []byte("already here"), 0o600)) + + err := copyStagedFile(context.Background(), strings.NewReader("new"), dest, 64) + require.Error(t, err) + assert.Contains(t, err.Error(), "creating the staged update binary") + + got, err := os.ReadFile(dest) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + assert.Equal(t, "already here", string(got)) +} + +func TestCopyStagedFile_ReadError(t *testing.T) { + t.Parallel() + + dest := filepath.Join(t.TempDir(), "zaparoo") + err := copyStagedFile(context.Background(), io.MultiReader( + strings.NewReader("some bytes"), + &failingReader{}, + ), dest, 1024) + + require.ErrorIs(t, err, ErrArchiveRejected) + assert.Contains(t, err.Error(), "extracting the update binary") +} + +// failingReader stands in for an archive member whose stream dies mid-read. +type failingReader struct{} + +func (*failingReader) Read([]byte) (int, error) { + return 0, io.ErrUnexpectedEOF +} + +// TestCopyIntoSink_WriteError is the failure the classification turns on: the +// bytes arriving are fine and the disk underneath is not. A device that has run +// out of space must not have the release condemned for it, so this error carries +// no verdict. +func TestCopyIntoSink_WriteError(t *testing.T) { + t.Parallel() + + sink := &failingSink{err: errDiskFull} + err := copyIntoSink(context.Background(), strings.NewReader(strings.Repeat("x", 64)), sink, 1024) + + require.Error(t, err) + require.NotErrorIs(t, err, ErrArchiveRejected) + require.ErrorIs(t, err, errDiskFull) + assert.Contains(t, err.Error(), "writing the staged update binary") + assert.True(t, sink.closed, "the destination has to be closed either way") +} + +// TestCopyIntoSink_ReadErrorStillRejects is the other half of the same switch: a +// stream that dies mid-member is the archive's problem and does earn a verdict. +func TestCopyIntoSink_ReadErrorStillRejects(t *testing.T) { + t.Parallel() + + sink := &failingSink{} + err := copyIntoSink(context.Background(), io.MultiReader( + strings.NewReader("some bytes"), + &failingReader{}, + ), sink, 1024) + + require.ErrorIs(t, err, ErrArchiveRejected) + assert.Contains(t, err.Error(), "extracting the update binary") +} + +// errDiskFull stands in for the write fault a full card produces, without +// needing a platform-specific errno. +var errDiskFull = errors.New("no space left on device") + +// failingSink stands in for a destination whose writes fail, which is how a full +// or failing card behaves. With err nil it accepts everything. +type failingSink struct { + err error + closed bool +} + +func (s *failingSink) Write(p []byte) (int, error) { + if s.err != nil { + return 0, s.err + } + return len(p), nil +} + +func (*failingSink) Sync() error { return nil } + +func (s *failingSink) Close() error { + s.closed = true + return nil +} diff --git a/pkg/service/updater/otameta/manifest.go b/pkg/service/updater/otameta/manifest.go index 739f5efa3..073607698 100644 --- a/pkg/service/updater/otameta/manifest.go +++ b/pkg/service/updater/otameta/manifest.go @@ -227,6 +227,18 @@ func isArchiveName(name, base string) bool { return false } +// ArchiveExtension returns the extension of an archive name, which is what +// decides how it is unpacked. The list of extensions lives here so the client +// and the publisher cannot disagree about what an update archive is. +func ArchiveExtension(name string) (string, error) { + for _, ext := range archiveExts { + if strings.HasSuffix(name, ext) { + return ext, nil + } + } + return "", fmt.Errorf("%w: %q is not an update archive", ErrNoAsset, name) +} + // FindRelease returns the release carrying a tag, or nil. func FindRelease(m *Manifest, tag string) *Release { if m == nil { diff --git a/pkg/service/updater/otameta/manifest_test.go b/pkg/service/updater/otameta/manifest_test.go index 7747964cb..2384a0516 100644 --- a/pkg/service/updater/otameta/manifest_test.go +++ b/pkg/service/updater/otameta/manifest_test.go @@ -390,3 +390,34 @@ func TestManifest_ChannelNames(t *testing.T) { assert.Equal(t, "beta", ChannelBeta) assert.True(t, strings.HasPrefix(ArchiveBaseName("linux", "amd64", "2.16.1"), "zaparoo-")) } + +func TestArchiveExtension(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + want string + }{ + {name: "zaparoo-mister_arm-2.16.1.zip", want: ".zip"}, + {name: "zaparoo-linux_amd64-2.16.1.tar.gz", want: ".tar.gz"}, + // .gz alone is not one of the two, so the suffix check must not treat the + // tail of .tar.gz as a match on its own. + {name: "zaparoo-linux_amd64-2.16.1.gz", want: ""}, + {name: "zaparoo-windows_amd64-2.16.1.exe", want: ""}, + {name: "checksums.txt", want: ""}, + {name: "", want: ""}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got, err := ArchiveExtension(tt.name) + if tt.want == "" { + require.ErrorIs(t, err, ErrNoAsset) + assert.Empty(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/service/updater/stage.go b/pkg/service/updater/stage.go new file mode 100644 index 000000000..79660ea27 --- /dev/null +++ b/pkg/service/updater/stage.go @@ -0,0 +1,706 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +package updater + +import ( + "context" + "crypto/sha256" + "crypto/subtle" + "encoding/hex" + "errors" + "fmt" + "io" + "net" + "net/http" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "time" + + "github.com/Masterminds/semver/v3" + "github.com/ZaparooProject/zaparoo-core/v2/pkg/config" + "github.com/ZaparooProject/zaparoo-core/v2/pkg/helpers/tlsroots" + "github.com/ZaparooProject/zaparoo-core/v2/pkg/service/updater/otameta" + "github.com/rs/zerolog/log" +) + +const ( + // maxArchiveBytes refuses an absurd declared archive size before a byte is + // read. Release archives are 10-16 MB today. + maxArchiveBytes = 256 << 20 + + // maxStagedFileBytes caps one file copied out of an archive. It is enforced + // against the bytes that actually arrive rather than the size the archive + // declares, so a lying header is caught mid-copy rather than trusted. The + // binary is around 45 MB uncompressed today. + maxStagedFileBytes = 256 << 20 + + // maxArchiveMembers bounds how much of an archive is walked. Releases carry + // the binary, a licence, a readme and on one platform a scripts directory, + // so a hundred is generous; the point is that the walk terminates. + maxArchiveMembers = 100 + + // maxArchiveInflatedBytes bounds the total content the walk reads out of one + // archive, across every member including the ones it skips. maxStagedFileBytes + // caps the file that is kept; this caps what getting to it can be made to + // cost. + maxArchiveInflatedBytes = 384 << 20 + + // downloadStallTimeout is how long a transfer may make no progress at all + // before it is abandoned. It deliberately bounds silence rather than total + // duration: a legitimate download to a MiSTer over a slow link can run for + // minutes, and killing that would leave those devices unable to update. + downloadStallTimeout = 90 * time.Second + + // stallChecks is how many times the stall guard looks per timeout. Checking + // more often than the timeout keeps the worst-case detection delay to a + // fraction of it rather than double. + stallChecks = 4 + + // probeTimeout bounds the staged binary's version check. It prints one line + // and exits before opening anything, so a binary that has not answered in + // ten seconds is not going to. + probeTimeout = 10 * time.Second + + // probeWaitDelay is how long the probe waits for the output pipes to close + // after the process itself is gone. Without it a leftover child holding those + // pipes would block the probe past its timeout, which would strand the whole + // update rather than fail it. + probeWaitDelay = 2 * time.Second + + // stagingRemoveAttempts and stagingRemoveDelay bound how long removing a + // staging directory is retried. A staging directory holds a binary this + // process may have just finished executing for the probe, and Windows can keep + // such a file open for a moment after the process itself is gone, which fails + // a single removal with a sharing violation. Retrying costs nothing on the + // platforms where the first attempt always works; on the one where it does + // not, it is the difference between a failed update leaving its whole payload + // on the disk and leaving nothing. + stagingRemoveAttempts = 20 + stagingRemoveDelay = 100 * time.Millisecond + + // stagingSubdir holds one directory per staged version, under the updater's + // own directory rather than anywhere the platform might clean up. + stagingSubdir = "staging" + + // payloadSubdir holds the files pulled out of the archive, all named by this + // package. + payloadSubdir = "payload" + + stagedFilePerm = 0o600 + stagedBinaryPerm = 0o755 +) + +var ( + // ErrNotAnUpgrade means the release is not newer than what is running. It is + // checked here rather than trusted from the check response because that is + // what stops a stale or tampered manifest installing an older build. + ErrNotAnUpgrade = errors.New("release is not newer than the running version") + + // ErrUpgradeFloor means the release declares a minimum version to come from + // and this device is below it, so it has to take an intermediate build + // first. + ErrUpgradeFloor = errors.New("release cannot be installed directly from the running version") + + // ErrArchiveRejected covers everything about the archive that fails a rule: + // a size the manifest will not vouch for, an unreadable or overlong member, + // a missing binary, or two of them. + ErrArchiveRejected = errors.New("release archive was rejected") + + // ErrChecksumMismatch means the bytes that arrived are not the bytes the + // signed manifest describes. + ErrChecksumMismatch = errors.New("release archive does not match the manifest checksum") + + // ErrDownloadStalled means the transfer stopped making progress, either + // because the stall guard saw no bytes for the whole stall timeout or because + // one of the transport's own deadlines ran out waiting for a connect, a + // handshake or the response headers. It is a fault of the network, never of + // the release. + ErrDownloadStalled = errors.New("release archive download stalled") + + // ErrProbeFailed means the staged binary would not run here, or ran and + // disagreed about what version it is. This is the check that keeps a bad + // build from reaching a device with no supervisor to recover it. + ErrProbeFailed = errors.New("staged binary failed its version probe") +) + +// StageOptions describes one staging attempt. +type StageOptions struct { + // Release comes from a manifest whose signature has already been checked. + // The archive and the version are re-derived from it here rather than + // trusting anything passed alongside it. + Release *otameta.Release + // PlatformID is the platform half of the archive name. + PlatformID string + // Arch and OS default to this build's. They are settable so the selection + // and the archive member rules can be tested for platforms other than the + // one running the test. + Arch string + OS string + // TargetPath is the binary that will eventually be replaced. Only its base + // name is read here: it names the archive member to pull out, and the name + // the staged copy is written under. + TargetPath string + // StagingRoot holds one directory per staged version. + StagingRoot string + // CurrentVersion is the version running now, which the release has to beat. + CurrentVersion string +} + +// StagedUpdate is a verified release unpacked into files this process named, +// ready for the install stage to move into place. Nothing outside Dir has been +// touched to produce it. +type StagedUpdate struct { + // Dir is the staging directory holding the archive and the payload. + // Removing it undoes the whole staging attempt. + Dir string + // BinaryPath is the new executable. It came out of an archive whose bytes on + // disk were checked against the signed manifest immediately before it was + // read, and it has answered a version probe on this device. + BinaryPath string + // ArchivePath is the downloaded archive, kept so a later stage can report + // what it installed from. + ArchivePath string + // Version is the release version, without the tag's leading v. + Version string +} + +// assetFetcher retrieves an asset URL. Production hands back the CDN response +// body; tests serve archives from a local server. +type assetFetcher func(ctx context.Context, target string) (io.ReadCloser, error) + +// stager holds the resolved settings for one staging attempt. The limits and +// timeouts are fields rather than constants read at the point of use so tests +// can drive the guards without a 256 MB fixture or a 90 second wait. +type stager struct { + fetch assetFetcher + release *otameta.Release + chmod func(string, os.FileMode) error + stagingRoot string + goarch string + binaryName string + goos string + current string + platformID string + maxFileBytes int64 + maxInflatedBytes int64 + stallTimeout time.Duration + probeTimeout time.Duration +} + +// stagingRootFor returns where staged versions live for a data directory. +func stagingRootFor(dataDir string) string { + dir := stateDirFor(dataDir) + if dir == "" { + return "" + } + return filepath.Join(dir, stagingSubdir) +} + +// Stage downloads a release, checks it against the signed manifest, pulls the +// binary out of it and proves that binary runs, without touching the live +// install. Every failure leaves the device exactly as it was. +func Stage(ctx context.Context, opts *StageOptions) (*StagedUpdate, error) { + // tlsroots hands back a transport this operation owns outright, so closing + // it at the end does not affect anything else in the process. + transport := tlsroots.Transport(nil) + transport.ResponseHeaderTimeout = responseHeaderTimeout + defer transport.CloseIdleConnections() + + return stageRelease(ctx, opts, assetFetcherFor(transport)) +} + +// stageRelease is Stage with the transfer injected, so tests can serve an +// archive without a network. +func stageRelease(ctx context.Context, opts *StageOptions, fetch assetFetcher) (*StagedUpdate, error) { + s, err := newStager(opts, fetch) + if err != nil { + return nil, err + } + return s.run(ctx) +} + +func newStager(opts *StageOptions, fetch assetFetcher) (*stager, error) { + if opts == nil { + return nil, errors.New("staging an update needs options") + } + if opts.Release == nil { + return nil, errors.New("staging an update needs a release") + } + if opts.PlatformID == "" { + return nil, errors.New("staging an update needs a platform id") + } + if opts.CurrentVersion == "" { + return nil, errors.New("staging an update needs the running version") + } + if opts.StagingRoot == "" { + return nil, errors.New("staging an update needs a staging directory") + } + if fetch == nil { + return nil, errors.New("staging an update needs a way to fetch the archive") + } + + // The base name is both the member to pull out and the name to write, so a + // target path that does not name a file has nothing to stage. + binaryName := filepath.Base(opts.TargetPath) + if opts.TargetPath == "" || binaryName == "." || binaryName == string(filepath.Separator) { + return nil, fmt.Errorf("staging an update needs the path of the binary to replace, got %q", opts.TargetPath) + } + + s := &stager{ + fetch: fetch, + release: opts.Release, + platformID: opts.PlatformID, + goos: opts.OS, + goarch: opts.Arch, + binaryName: binaryName, + stagingRoot: opts.StagingRoot, + current: opts.CurrentVersion, + maxFileBytes: maxStagedFileBytes, + maxInflatedBytes: maxArchiveInflatedBytes, + stallTimeout: downloadStallTimeout, + probeTimeout: probeTimeout, + chmod: os.Chmod, + } + if s.goos == "" { + s.goos = runtime.GOOS + } + if s.goarch == "" { + s.goarch = runtime.GOARCH + } + return s, nil +} + +func (s *stager) run(ctx context.Context) (*StagedUpdate, error) { + asset, version, err := s.selectArchive() + if err != nil { + return nil, err + } + + // The version has already been through semver parsing, which admits only + // digits, dots, hyphens and alphanumerics, so it cannot name anything but a + // single directory. Asserted rather than argued about, because it is the one + // string out of the manifest that becomes a path. + if version == "." || version == ".." || version != filepath.Base(version) { + return nil, fmt.Errorf("%w: %q cannot name a staging directory", ErrArchiveRejected, version) + } + dir := filepath.Join(s.stagingRoot, version) + + // A previous attempt that died without cleaning up would otherwise collide + // with this one's exclusive file creation. + if rmErr := removeStagingDir(dir); rmErr != nil { + return nil, fmt.Errorf("clearing previous update staging directory: %w", rmErr) + } + pruneStagingRoot(s.stagingRoot, version) + //nolint:gosec // G703: the version is asserted above to be a single path element + if mkErr := os.MkdirAll(dir, stateDirPerm); mkErr != nil { + return nil, fmt.Errorf("creating update staging directory: %w", mkErr) + } + + staged, err := s.stageInto(ctx, dir, asset, version) + if err != nil { + // Nothing outside this directory has been written, so discarding it + // leaves no trace of the attempt. + if rmErr := removeStagingDir(dir); rmErr != nil { + log.Warn().Err(rmErr).Str("dir", dir).Msg("could not remove failed update staging directory") + } + return nil, err + } + + log.Info(). + Str("version", staged.Version). + Str("binary", staged.BinaryPath). + Msg("staged update is verified and runnable") + return staged, nil +} + +// pruneStagingRoot deletes every staged version except the one being staged now. +// +// The failure path below removes this attempt's own directory, but it only runs +// when the process lives long enough to reach it. A power cut or a kill during a +// download leaves a directory that nothing afterwards is looking for: the next +// release computes a different name, so without this the whole ~60 MB of a +// half-staged version stays on the SD card for the life of the device, next to +// the config and the media database. Sweeping on the way in rather than on the +// way out means an orphan is collected by the next attempt whatever killed the +// last one. +// +// Failures are logged and not returned. Being unable to tidy up is not a reason +// to refuse an update. +func pruneStagingRoot(root, keep string) { + entries, err := os.ReadDir(root) + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + log.Warn().Err(err).Str("dir", root).Msg("could not read the update staging directory") + } + return + } + + for _, entry := range entries { + if entry.Name() == keep { + continue + } + stale := filepath.Join(root, entry.Name()) + if rmErr := removeStagingDir(stale); rmErr != nil { + log.Warn().Err(rmErr).Str("dir", stale).Msg("could not remove an orphaned update staging directory") + continue + } + log.Info().Str("dir", stale).Msg("removed an orphaned update staging directory") + } +} + +// removeStagingDir deletes a staging directory, retrying for a bounded time. +// +// See stagingRemoveAttempts for why one attempt is not enough. Sleeping only +// happens when a removal has actually failed, so the ordinary path is a single +// call. +func removeStagingDir(dir string) error { + var err error + for attempt := range stagingRemoveAttempts { + if attempt > 0 { + time.Sleep(stagingRemoveDelay) + } + //nolint:gosec // G703: callers pass a path this package built under its own staging root + if err = os.RemoveAll(dir); err == nil { + return nil + } + } + return fmt.Errorf("removing update staging directory %q: %w", dir, err) +} + +func (s *stager) stageInto( + ctx context.Context, dir string, asset *otameta.Asset, version string, +) (*StagedUpdate, error) { + ext, err := otameta.ArchiveExtension(asset.Name) + if err != nil { + return nil, fmt.Errorf("%w: %w", ErrArchiveRejected, err) + } + wantDigest, err := assetDigest(asset) + if err != nil { + return nil, err + } + + // The archive is written under a name built from the version and extension + // rather than the one the manifest gives it, so no metadata string reaches + // the filesystem even though selection has already constrained it. + archivePath := filepath.Join(dir, otameta.ArchiveBaseName(s.platformID, s.goarch, version)+ext) + if err := s.downloadArchive(ctx, asset, archivePath); err != nil { + return nil, err + } + + payloadDir := filepath.Join(dir, payloadSubdir) + //nolint:gosec // G703: dir is the staging directory this package created, payloadSubdir is a constant + if err := os.MkdirAll(payloadDir, stateDirPerm); err != nil { + return nil, fmt.Errorf("creating update payload directory: %w", err) + } + + binaryPath := filepath.Join(payloadDir, s.binaryName) + if err := s.extractBinary(ctx, archivePath, ext, wantDigest, binaryPath); err != nil { + return nil, err + } + + // Extraction is a long uninterruptible stretch on a slow device, so the probe + // is only meaningful if the caller is still interested in the answer. Without + // this, a shutdown part-way through staging would run the probe on a dead + // context and report a perfectly good build as unrunnable. + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("staging the update was cancelled: %w", err) + } + + // The exec bit is set here rather than taken from the archive, so an archive + // cannot decide what is executable. + // + // A failure here is not fatal, because on the volumes MiSTer and MiSTeX + // install to it does not mean what it looks like it means. /media/fat is vfat + // or exFAT, which has no mode bits: the exec bit comes from the mount's mask, + // and chmod is either a silent no-op or an outright error depending on which + // driver mounted it. In the error case the file is already executable and + // refusing here would reject a release that runs perfectly well. The probe + // below is what actually decides whether this binary can execute, so let it, + // and record that we could not set the bit ourselves in case the probe then + // fails for a reason that needs this context. + //nolint:gosec // an executable has to be executable; the archive does not get a say in it + if err := s.chmod(binaryPath, stagedBinaryPerm); err != nil { + log.Warn().Err(err). + Str("binary", binaryPath). + Msg("could not set the exec bit on the staged binary; leaving it to the probe") + } + + if err := s.probeBinary(ctx, binaryPath, version); err != nil { + return nil, err + } + + return &StagedUpdate{ + Dir: dir, + BinaryPath: binaryPath, + ArchivePath: archivePath, + Version: version, + }, nil +} + +// selectArchive picks the one archive this device may install from the release +// and refuses anything that is not a step forward. The version assertions are +// made here, against the release's own tag, because the check that offered the +// update is a separate operation whose answer this one does not take on trust. +func (s *stager) selectArchive() (*otameta.Asset, string, error) { + if s.release.Draft { + return nil, "", fmt.Errorf("%w: %s is a draft", ErrArchiveRejected, s.release.TagName) + } + + version := otameta.VersionFromTag(s.release.TagName) + target, err := semver.NewVersion(version) + if err != nil { + return nil, "", fmt.Errorf("%w: release %q has no usable version: %w", + ErrArchiveRejected, s.release.TagName, err) + } + current, err := semver.NewVersion(s.current) + if err != nil { + return nil, "", fmt.Errorf("reading the running version %q: %w", s.current, err) + } + + if !target.GreaterThan(current) { + return nil, "", fmt.Errorf("%w: %s is not newer than %s", ErrNotAnUpgrade, version, s.current) + } + + if s.release.MinUpgradeFrom != "" { + floor, floorErr := semver.NewVersion(s.release.MinUpgradeFrom) + if floorErr != nil { + return nil, "", fmt.Errorf("%w: %s declares an unusable min_upgrade_from %q: %w", + ErrArchiveRejected, s.release.TagName, s.release.MinUpgradeFrom, floorErr) + } + if current.LessThan(floor) { + return nil, "", fmt.Errorf("%w: %s needs %s or newer first, running %s", + ErrUpgradeFloor, version, s.release.MinUpgradeFrom, s.current) + } + } + + asset, err := otameta.SelectAsset(s.release, s.platformID, s.goarch) + if err != nil { + return nil, "", fmt.Errorf("selecting the update archive: %w", err) + } + return asset, version, nil +} + +// downloadArchive streams the archive to disk, hashing as it goes, and accepts +// it only if both the length and the digest are exactly what the signed +// manifest says. The body is never read into memory. +func (s *stager) downloadArchive(ctx context.Context, asset *otameta.Asset, dest string) error { + if asset.Size <= 0 { + return fmt.Errorf("%w: the manifest declares no size for %s", ErrArchiveRejected, asset.Name) + } + if asset.Size > maxArchiveBytes { + return fmt.Errorf("%w: the manifest declares %d bytes for %s, over the %d byte limit", + ErrArchiveRejected, asset.Size, asset.Name, maxArchiveBytes) + } + want, err := assetDigest(asset) + if err != nil { + return err + } + + stallCtx, guard := newStallGuard(ctx, s.stallTimeout) + defer guard.stop() + + body, err := s.fetch(stallCtx, asset.URL) + if err != nil { + // Classified the same way a mid-body failure is. Before the first byte the + // transport usually decides first: the dial, the TLS handshake and the + // response headers each carry a deadline of their own, and all three are + // shorter than the stall timeout, so classifyTransferErr maps a timeout + // from any of them onto the same stall verdict. The guard covers the case + // none of those deadlines can see, where every hop of a redirect chain + // answers inside its own budget and the transfer as a whole still goes + // nowhere. + return s.classifyTransferErr(ctx, guard, err, 0) + } + defer closeQuietly(body, "update archive response") + + //nolint:gosec // the path is built by this package inside its own staging directory + f, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, stagedFilePerm) + if err != nil { + return fmt.Errorf("creating the update archive file: %w", err) + } + + // One byte past the declared size, so an archive of exactly that length is + // accepted and a longer one is detected rather than silently truncated into + // something that would then fail the digest for the wrong reason. + digest := sha256.New() + written, copyErr := io.Copy(io.MultiWriter(f, digest), guard.reader(io.LimitReader(body, asset.Size+1))) + syncErr := f.Sync() + closeErr := f.Close() + + switch { + case copyErr != nil: + return s.classifyTransferErr(ctx, guard, copyErr, written) + case syncErr != nil: + return fmt.Errorf("flushing the update archive to disk: %w", syncErr) + case closeErr != nil: + return fmt.Errorf("closing the update archive: %w", closeErr) + } + + if written != asset.Size { + return fmt.Errorf("%w: %s is %d bytes, the manifest declares %d", + ErrArchiveRejected, asset.Name, written, asset.Size) + } + if subtle.ConstantTimeCompare(digest.Sum(nil), want) != 1 { + return fmt.Errorf("%w: %s hashes to %s, the manifest declares %s", + ErrChecksumMismatch, asset.Name, hex.EncodeToString(digest.Sum(nil)), asset.SHA256) + } + + log.Debug().Str("archive", asset.Name).Int64("bytes", written).Msg("update archive verified") + return nil +} + +// assetDigest decodes the digest the signed manifest gives for an asset. Both +// the download and the read that feeds extraction check against it, so it is +// decoded in one place. +func assetDigest(asset *otameta.Asset) ([]byte, error) { + want, err := hex.DecodeString(asset.SHA256) + if err != nil || len(want) != sha256.Size { + return nil, fmt.Errorf("%w: the manifest has no usable sha256 for %s", ErrArchiveRejected, asset.Name) + } + return want, nil +} + +// classifyTransferErr tells the three ways a transfer can fail apart. Both the +// request and the body read cancel through the same guarded context, so the +// error either side hands back says only "cancelled" and the reason has to come +// from which of the two cancelled it. Caller intent is checked first: a caller +// who gave up mid-stall is not reporting a network fault. +func (s *stager) classifyTransferErr( + ctx context.Context, guard *stallGuard, err error, written int64, +) error { + var netErr net.Error + switch { + case ctx.Err() != nil: + return fmt.Errorf("update archive download was cancelled after %d bytes: %w", written, ctx.Err()) + case guard.tripped(): + return fmt.Errorf("%w after %d bytes with no progress for %s", + ErrDownloadStalled, written, s.stallTimeout) + case errors.As(err, &netErr) && netErr.Timeout(): + // A deadline the transport owns rather than one the guard watches: the + // connect, the handshake or the response headers gave up. Silence the + // guard never gets to see, but silence all the same, and dead versus slow + // is the distinction this sentinel exists to draw. + return fmt.Errorf("%w after %d bytes: %w", ErrDownloadStalled, written, err) + default: + return fmt.Errorf("downloading the update archive: %w", err) + } +} + +// probeBinary runs the staged binary's version flag. It is the load-bearing +// check for the platforms with no supervisor: the binary has to demonstrably +// execute here, and agree about what it is, before anything replaces the one +// that is currently working. It catches a wrong architecture, a libc mismatch, +// a missing shared library, an exec bit a vfat mount dropped, a noexec mount, +// and a version that disagrees with the manifest. +func (s *stager) probeBinary(ctx context.Context, binaryPath, version string) error { + probeCtx, cancel := context.WithTimeout(ctx, s.probeTimeout) + defer cancel() + + //nolint:gosec // the path is a file this process just created inside its own staging directory + cmd := exec.CommandContext(probeCtx, binaryPath, "-"+config.VersionFlagName) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + // Killing the process is not enough to unblock Wait if it left a child + // holding these pipes. This runs a binary that was downloaded seconds ago, + // so the timeout has to bound the call and not just the process. + cmd.WaitDelay = probeWaitDelay + + runErr := cmd.Run() + if runErr != nil { + switch { + case ctx.Err() != nil: + // The caller gave up. Reporting that as a failed probe would + // condemn a build that was never actually judged. + return fmt.Errorf("update probe was cancelled: %w", ctx.Err()) + case errors.Is(probeCtx.Err(), context.DeadlineExceeded): + return fmt.Errorf("%w: no answer within %s", ErrProbeFailed, s.probeTimeout) + default: + return fmt.Errorf("%w: %w (stderr: %s)", ErrProbeFailed, runErr, clip(stderr.String(), 256)) + } + } + + // One matching line, not the whole stream: the comparison is made by the + // build that is already installed against one that did not exist when it + // shipped, so a future release that prints something extra alongside its + // version must not be judged unrunnable for it. + want := config.VersionLine(version, s.platformID) + if !hasLine(stdout.String(), want) { + return fmt.Errorf("%w: printed %q, expected a line reading %q", + ErrProbeFailed, clip(stdout.String(), 256), want) + } + + log.Debug().Str("binary", binaryPath).Msg("staged binary answered its version probe") + return nil +} + +// assetFetcherFor returns a fetcher backed by an HTTP transport. +func assetFetcherFor(transport *http.Transport) assetFetcher { + return func(ctx context.Context, target string) (io.ReadCloser, error) { + // No client deadline: the archive is the one response whose size is not + // bounded by a small constant, so total duration is the caller's context + // to bound and the stall guard is what tells slow apart from dead. + client := &http.Client{Transport: transport} + req, err := http.NewRequestWithContext(ctx, http.MethodGet, target, http.NoBody) + if err != nil { + return nil, fmt.Errorf("creating the update archive request: %w", err) + } + + res, err := client.Do(req) //nolint:bodyclose // the body is the return value; the caller closes it + if err != nil { + return nil, fmt.Errorf("requesting the update archive: %w", err) + } + if res.StatusCode != http.StatusOK { + closeQuietly(res.Body, "update archive response") + return nil, fmt.Errorf("the update archive request failed with status %d", res.StatusCode) + } + return res.Body, nil + } +} + +func closeQuietly(c io.Closer, what string) { + if err := c.Close(); err != nil { + log.Debug().Err(err).Msgf("closing %s", what) + } +} + +// hasLine reports whether any line of out is exactly want. A trailing carriage +// return is ignored so output that has been through a CRLF channel still +// matches. +func hasLine(out, want string) bool { + for line := range strings.SplitSeq(out, "\n") { + if strings.TrimSuffix(line, "\r") == want { + return true + } + } + return false +} + +// clip shortens a string for an error message. +func clip(s string, limit int) string { + trimmed := strings.TrimSpace(s) + if len(trimmed) <= limit { + return trimmed + } + return trimmed[:limit] + "…" +} diff --git a/pkg/service/updater/stage_test.go b/pkg/service/updater/stage_test.go new file mode 100644 index 000000000..245fae929 --- /dev/null +++ b/pkg/service/updater/stage_test.go @@ -0,0 +1,1165 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +package updater + +import ( + "bytes" + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "runtime" + "strconv" + "strings" + "testing" + "time" + + "github.com/ZaparooProject/zaparoo-core/v2/pkg/config" + "github.com/ZaparooProject/zaparoo-core/v2/pkg/service/updater/otameta" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // testStageVersion is compiled into the fake binary, so every test that + // reaches the probe stages this version. + testStageVersion = "2.11.0" + testStagePlatform = "linux" + testStageArch = "amd64" + testCurrentVersion = "2.10.1" +) + +// fakeBinaryTemplate is a stand-in release binary. It answers the version probe +// the way the real one does, and picks a way to misbehave from its own file +// name, which is the only channel available: the probe inherits this process's +// environment and staging deletes anything placed alongside the binary, so +// neither an env var nor a sidecar file can select a behaviour per test. +const fakeBinaryTemplate = `package main + +import ( + "os" + "path/filepath" + "strings" + "time" +) + +func main() { + switch strings.ToLower(strings.TrimSuffix(filepath.Base(os.Args[0]), ".exe")) { + case "zaparoo-fail": + os.Stderr.WriteString("error while loading shared libraries: libz.so.1\n") + os.Exit(1) + case "zaparoo-wrong": + os.Stdout.WriteString(__WRONG__) + case "zaparoo-chatty": + os.Stderr.WriteString("warning: config file not found, using defaults\n") + os.Stdout.WriteString("some future build says something here first\n") + os.Stdout.WriteString(__GOOD__) + case "zaparoo-hang": + time.Sleep(10 * time.Minute) + default: + os.Stdout.WriteString(__GOOD__) + } +} +` + +var ( + fakeBinaryPath string + errFakeBinary error +) + +func TestMain(m *testing.M) { + dir, err := os.MkdirTemp("", "zaparoo-updater-fake") + if err == nil { + defer func() { _ = os.RemoveAll(dir) }() + fakeBinaryPath, errFakeBinary = buildFakeBinary(dir) + } else { + errFakeBinary = err + } + m.Run() +} + +// buildFakeBinary compiles the stand-in release binary once for the package. +// Nothing else can honestly test the probe: it has to be a real executable, +// and the failures worth catching are a process that will not start, one that +// answers with the wrong version, and one that never answers. +func buildFakeBinary(dir string) (string, error) { + // Built from config.VersionLine rather than a literal of its own, so this + // fixture cannot pass while a real release binary would fail the probe. + source := strings.NewReplacer( + "__GOOD__", strconv.Quote(config.VersionLine(testStageVersion, testStagePlatform)+"\n"), + "__WRONG__", strconv.Quote(config.VersionLine("0.0.1", testStagePlatform)+"\n"), + ).Replace(fakeBinaryTemplate) + + if err := os.WriteFile(filepath.Join(dir, "main.go"), []byte(source), 0o600); err != nil { + return "", fmt.Errorf("writing the fake binary source: %w", err) + } + gomod := "module zaparoofake\n\ngo 1.21\n" + if err := os.WriteFile(filepath.Join(dir, "go.mod"), []byte(gomod), 0o600); err != nil { + return "", fmt.Errorf("writing the fake binary go.mod: %w", err) + } + + out := filepath.Join(dir, testBinaryName("fake")) + //nolint:gosec // a fixed command compiling a source file this test just wrote + cmd := exec.CommandContext(context.Background(), "go", "build", "-o", out, ".") + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GOWORK=off", "GOFLAGS=", "CGO_ENABLED=0") + if combined, buildErr := cmd.CombinedOutput(); buildErr != nil { + return "", fmt.Errorf("building the fake binary: %w (%s)", buildErr, combined) + } + return out, nil +} + +// testBinaryName adds the extension Windows needs. Without it exec cannot find +// the staged file at all, because Windows resolves even an absolute path +// through PATHEXT. +func testBinaryName(stem string) string { + if runtime.GOOS == "windows" { + return stem + ".exe" + } + return stem +} + +func fakeBinaryBytes(t *testing.T) []byte { + t.Helper() + + require.NoError(t, errFakeBinary, "the fake release binary did not build") + body, err := os.ReadFile(fakeBinaryPath) //nolint:gosec // built by this package into its own temp dir + require.NoError(t, err) + return body +} + +// releaseArchive builds an archive shaped like a real release: the binary plus +// the licence and readme that must not be extracted. +func releaseArchive(t *testing.T, ext, memberName string, binary []byte) []byte { + t.Helper() + + path := filepath.Join(t.TempDir(), "release"+ext) + switch ext { + case archiveExtTarGz: + writeTarGz(t, path, []tarMember{ + {name: "LICENSE.txt", body: []byte("gpl")}, + {name: "README.txt", body: []byte("readme")}, + {name: memberName, body: binary, mode: 0o755}, + }) + case archiveExtZip: + writeZip(t, path, []zipMember{ + {name: "LICENSE.txt", body: []byte("gpl")}, + {name: "README.txt", body: []byte("readme")}, + {name: memberName, body: binary, mode: 0o755}, + }) + default: + t.Fatalf("unsupported archive extension %q", ext) + } + + body, err := os.ReadFile(path) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + return body +} + +// testArchiveName is the only name a release may publish an archive under for +// this platform and version, which is what binds the archive to the version. +func testArchiveName(version, ext string) string { + return otameta.ArchiveBaseName(testStagePlatform, testStageArch, version) + ext +} + +// servedAsset publishes body over HTTP and describes it exactly as a verified +// manifest would. +func servedAsset(t *testing.T, name string, body []byte) *otameta.Asset { + t.Helper() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + _, _ = w.Write(body) + })) + t.Cleanup(srv.Close) + + sum := sha256.Sum256(body) + return &otameta.Asset{ + Name: name, + URL: srv.URL + "/" + name, + SHA256: hex.EncodeToString(sum[:]), + Size: int64(len(body)), + } +} + +func testRelease(tag string, assets ...*otameta.Asset) *otameta.Release { + return &otameta.Release{ + Name: tag, + TagName: tag, + Channel: otameta.ChannelStable, + Assets: assets, + Rollout: 100, + } +} + +func testAssetFetcher(t *testing.T) assetFetcher { + t.Helper() + + transport := &http.Transport{} + t.Cleanup(transport.CloseIdleConnections) + return assetFetcherFor(transport) +} + +// unusedFetcher fails loudly, for the stages that must decide before any bytes +// are requested. +func unusedFetcher(context.Context, string) (io.ReadCloser, error) { + return nil, errors.New("the archive should not have been fetched") +} + +func testStageOptions(t *testing.T, rel *otameta.Release, stem string) *StageOptions { + t.Helper() + + root := t.TempDir() + return &StageOptions{ + Release: rel, + PlatformID: testStagePlatform, + Arch: testStageArch, + // Pinned rather than taken from the host, so one set of fixtures covers + // the archive member rules whatever this test is running on. + OS: "linux", + TargetPath: filepath.Join(root, "install", testBinaryName(stem)), + StagingRoot: filepath.Join(root, "updater", stagingSubdir), + CurrentVersion: testCurrentVersion, + } +} + +func TestNewStager_RequiresItsInputs(t *testing.T) { + t.Parallel() + + valid := &StageOptions{ + Release: testRelease("v2.11.0"), + PlatformID: testStagePlatform, + TargetPath: filepath.Join("install", "zaparoo"), + StagingRoot: filepath.Join("data", "updater", "staging"), + CurrentVersion: testCurrentVersion, + } + + tests := []struct { + mutate func(*StageOptions) + name string + wantMsg string + noFetch bool + }{ + { + name: "no release", + mutate: func(o *StageOptions) { o.Release = nil }, + wantMsg: "needs a release", + }, + { + name: "no platform", + mutate: func(o *StageOptions) { o.PlatformID = "" }, + wantMsg: "needs a platform id", + }, + { + name: "no running version", + mutate: func(o *StageOptions) { o.CurrentVersion = "" }, + wantMsg: "needs the running version", + }, + { + name: "no staging root", + mutate: func(o *StageOptions) { o.StagingRoot = "" }, + wantMsg: "needs a staging directory", + }, + { + name: "no target path", + mutate: func(o *StageOptions) { o.TargetPath = "" }, + wantMsg: "needs the path of the binary to replace", + }, + { + name: "target path is a directory", + mutate: func(o *StageOptions) { o.TargetPath = "." }, + wantMsg: "needs the path of the binary to replace", + }, + { + name: "no fetcher", + mutate: func(*StageOptions) {}, + noFetch: true, + wantMsg: "needs a way to fetch the archive", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + opts := *valid + tt.mutate(&opts) + + fetch := assetFetcher(unusedFetcher) + if tt.noFetch { + fetch = nil + } + + s, err := newStager(&opts, fetch) + require.Error(t, err) + assert.Nil(t, s) + assert.Contains(t, err.Error(), tt.wantMsg) + }) + } +} + +func TestNewStager_DefaultsToThisBuild(t *testing.T) { + t.Parallel() + + s, err := newStager(&StageOptions{ + Release: testRelease("v2.11.0"), + PlatformID: testStagePlatform, + TargetPath: filepath.Join("install", "zaparoo"), + StagingRoot: filepath.Join("data", "updater", "staging"), + CurrentVersion: testCurrentVersion, + }, unusedFetcher) + + require.NoError(t, err) + assert.Equal(t, runtime.GOOS, s.goos) + assert.Equal(t, runtime.GOARCH, s.goarch) + assert.Equal(t, "zaparoo", s.binaryName) +} + +func TestStage_ValidatesBeforeTouchingTheNetwork(t *testing.T) { + t.Parallel() + + staged, err := Stage(context.Background(), &StageOptions{}) + require.Error(t, err) + assert.Nil(t, staged) +} + +// TestSelectArchive covers the assertions that make a stale or tampered +// manifest unable to install something older than what is running. They are +// made here against the release's own tag rather than trusted from whatever +// offered the update. +func TestSelectArchive(t *testing.T) { + t.Parallel() + + asset := func(version string) *otameta.Asset { + return &otameta.Asset{Name: testArchiveName(version, archiveExtTarGz), Size: 1024} + } + + tests := []struct { + wantErr error + build func() *otameta.Release + name string + current string + wantMsg string + wantVersion string + }{ + { + name: "newer release", + build: func() *otameta.Release { + return testRelease("v2.11.0", asset("2.11.0")) + }, + current: "2.10.1", + wantVersion: "2.11.0", + }, + { + name: "draft", + build: func() *otameta.Release { + rel := testRelease("v2.11.0", asset("2.11.0")) + rel.Draft = true + return rel + }, + current: "2.10.1", + wantErr: ErrArchiveRejected, + wantMsg: "is a draft", + }, + { + name: "same version", + build: func() *otameta.Release { + return testRelease("v2.10.1", asset("2.10.1")) + }, + current: "2.10.1", + wantErr: ErrNotAnUpgrade, + }, + { + name: "older version", + build: func() *otameta.Release { + return testRelease("v2.9.0", asset("2.9.0")) + }, + current: "2.10.1", + wantErr: ErrNotAnUpgrade, + }, + { + name: "prerelease of the running version is not an upgrade", + build: func() *otameta.Release { + return testRelease("v2.10.1-beta.1", asset("2.10.1-beta.1")) + }, + current: "2.10.1", + wantErr: ErrNotAnUpgrade, + }, + { + name: "unusable release version", + build: func() *otameta.Release { + return testRelease("vnot-a-version", asset("not-a-version")) + }, + current: "2.10.1", + wantErr: ErrArchiveRejected, + wantMsg: "no usable version", + }, + { + name: "unusable running version", + build: func() *otameta.Release { + return testRelease("v2.11.0", asset("2.11.0")) + }, + current: "not-a-version", + wantMsg: "reading the running version", + }, + { + name: "upgrade floor above the running version", + build: func() *otameta.Release { + rel := testRelease("v2.11.0", asset("2.11.0")) + rel.MinUpgradeFrom = "2.10.2" + return rel + }, + current: "2.10.1", + wantErr: ErrUpgradeFloor, + wantMsg: "needs 2.10.2 or newer first", + }, + { + name: "upgrade floor equal to the running version", + build: func() *otameta.Release { + rel := testRelease("v2.11.0", asset("2.11.0")) + rel.MinUpgradeFrom = "2.10.1" + return rel + }, + current: "2.10.1", + wantVersion: "2.11.0", + }, + { + name: "upgrade floor below the running version", + build: func() *otameta.Release { + rel := testRelease("v2.11.0", asset("2.11.0")) + rel.MinUpgradeFrom = "2.6.0" + return rel + }, + current: "2.10.1", + wantVersion: "2.11.0", + }, + { + name: "unusable upgrade floor", + build: func() *otameta.Release { + rel := testRelease("v2.11.0", asset("2.11.0")) + rel.MinUpgradeFrom = "sometime" + return rel + }, + current: "2.10.1", + wantErr: ErrArchiveRejected, + wantMsg: "unusable min_upgrade_from", + }, + // A manifest claiming a high version while carrying a genuine older + // archive selects nothing: the candidate name is built from the tag, so + // relabelling the release stops it matching its own assets. + { + name: "relabelled release does not match its own archives", + build: func() *otameta.Release { + return testRelease("v99.0.0", asset("2.10.1")) + }, + current: "2.10.1", + wantErr: otameta.ErrNoAsset, + }, + { + name: "no archive for this platform", + build: func() *otameta.Release { + return testRelease("v2.11.0", &otameta.Asset{ + Name: otameta.ArchiveBaseName("windows", "amd64", "2.11.0") + archiveExtZip, + }) + }, + current: "2.10.1", + wantErr: otameta.ErrNoAsset, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + opts := testStageOptions(t, tt.build(), "zaparoo") + opts.CurrentVersion = tt.current + s, err := newStager(opts, unusedFetcher) + require.NoError(t, err) + + got, version, err := s.selectArchive() + if tt.wantVersion != "" { + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, tt.wantVersion, version) + assert.Equal(t, testArchiveName(tt.wantVersion, archiveExtTarGz), got.Name) + return + } + + require.Error(t, err) + assert.Empty(t, version) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + } + if tt.wantMsg != "" { + assert.Contains(t, err.Error(), tt.wantMsg) + } + }) + } +} + +func TestSelectArchive_ArmDoesNotPickUpArm64(t *testing.T) { + t.Parallel() + + rel := testRelease("v2.11.0", &otameta.Asset{ + Name: otameta.ArchiveBaseName("mister", "arm64", "2.11.0") + archiveExtZip, + }) + opts := testStageOptions(t, rel, "zaparoo.sh") + opts.PlatformID = "mister" + opts.Arch = "arm" + + s, err := newStager(opts, unusedFetcher) + require.NoError(t, err) + + _, _, err = s.selectArchive() + require.ErrorIs(t, err, otameta.ErrNoAsset) +} + +func TestDownloadArchive(t *testing.T) { + t.Parallel() + + body := bytes.Repeat([]byte("zaparoo release archive"), 64) + sum := sha256.Sum256(body) + + tests := []struct { + wantErr error + mutate func(*otameta.Asset) + name string + wantMsg string + }{ + { + name: "accepts what the manifest describes", + mutate: func(*otameta.Asset) {}, + }, + { + name: "no declared size", + mutate: func(a *otameta.Asset) { a.Size = 0 }, + wantErr: ErrArchiveRejected, + wantMsg: "declares no size", + }, + { + name: "negative declared size", + mutate: func(a *otameta.Asset) { a.Size = -1 }, + wantErr: ErrArchiveRejected, + wantMsg: "declares no size", + }, + { + name: "declared size over the cap", + mutate: func(a *otameta.Asset) { a.Size = maxArchiveBytes + 1 }, + wantErr: ErrArchiveRejected, + wantMsg: "over the", + }, + { + name: "unusable digest", + mutate: func(a *otameta.Asset) { a.SHA256 = "not hex" }, + wantErr: ErrArchiveRejected, + wantMsg: "no usable sha256", + }, + { + name: "digest of the wrong length", + mutate: func(a *otameta.Asset) { a.SHA256 = hex.EncodeToString(sum[:16]) }, + wantErr: ErrArchiveRejected, + wantMsg: "no usable sha256", + }, + { + name: "digest does not match the bytes", + mutate: func(a *otameta.Asset) { a.SHA256 = strings.Repeat("ab", sha256.Size) }, + wantErr: ErrChecksumMismatch, + }, + { + name: "shorter than declared", + mutate: func(a *otameta.Asset) { a.Size = int64(len(body)) + 10 }, + wantErr: ErrArchiveRejected, + wantMsg: "the manifest declares", + }, + { + name: "longer than declared", + mutate: func(a *otameta.Asset) { a.Size = int64(len(body)) - 10 }, + wantErr: ErrArchiveRejected, + wantMsg: "the manifest declares", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + asset := servedAsset(t, testArchiveName(testStageVersion, archiveExtTarGz), body) + tt.mutate(asset) + + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + s, err := newStager(opts, testAssetFetcher(t)) + require.NoError(t, err) + + dest := filepath.Join(t.TempDir(), asset.Name) + err = s.downloadArchive(context.Background(), asset, dest) + + if tt.wantErr == nil && tt.wantMsg == "" { + require.NoError(t, err) + got, readErr := os.ReadFile(dest) //nolint:gosec // test path under t.TempDir + require.NoError(t, readErr) + assert.Equal(t, body, got) + return + } + + require.Error(t, err) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + } + if tt.wantMsg != "" { + assert.Contains(t, err.Error(), tt.wantMsg) + } + }) + } +} + +func TestDownloadArchive_ServerError(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + + asset := &otameta.Asset{ + Name: testArchiveName(testStageVersion, archiveExtTarGz), + URL: srv.URL + "/missing", + SHA256: strings.Repeat("00", sha256.Size), + Size: 1024, + } + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + s, err := newStager(opts, testAssetFetcher(t)) + require.NoError(t, err) + + dest := filepath.Join(t.TempDir(), asset.Name) + err = s.downloadArchive(context.Background(), asset, dest) + require.Error(t, err) + assert.Contains(t, err.Error(), "404") + assert.NoFileExists(t, dest) +} + +// TestDownloadArchive_Stalls proves the guard bounds silence rather than total +// duration: the server answers, sends a little, and then never sends again. +func TestDownloadArchive_Stalls(t *testing.T) { + t.Parallel() + + body := bytes.Repeat([]byte("x"), 4096) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + _, _ = w.Write(body[:8]) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + <-r.Context().Done() + })) + t.Cleanup(srv.Close) + + sum := sha256.Sum256(body) + asset := &otameta.Asset{ + Name: testArchiveName(testStageVersion, archiveExtTarGz), + URL: srv.URL + "/archive", + SHA256: hex.EncodeToString(sum[:]), + Size: int64(len(body)), + } + + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + s, err := newStager(opts, testAssetFetcher(t)) + require.NoError(t, err) + s.stallTimeout = 200 * time.Millisecond + + dest := filepath.Join(t.TempDir(), asset.Name) + err = s.downloadArchive(context.Background(), asset, dest) + require.ErrorIs(t, err, ErrDownloadStalled) + assert.Contains(t, err.Error(), "no progress") +} + +func TestDownloadArchive_CallerCancels(t *testing.T) { + t.Parallel() + + body := bytes.Repeat([]byte("x"), 4096) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", strconv.Itoa(len(body))) + _, _ = w.Write(body[:8]) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + <-r.Context().Done() + })) + t.Cleanup(srv.Close) + + sum := sha256.Sum256(body) + asset := &otameta.Asset{ + Name: testArchiveName(testStageVersion, archiveExtTarGz), + URL: srv.URL + "/archive", + SHA256: hex.EncodeToString(sum[:]), + Size: int64(len(body)), + } + + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + s, err := newStager(opts, testAssetFetcher(t)) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(100 * time.Millisecond) + cancel() + }() + defer cancel() + + dest := filepath.Join(t.TempDir(), asset.Name) + err = s.downloadArchive(ctx, asset, dest) + require.Error(t, err) + // A caller giving up is not a stall, and must not be reported as one. + require.NotErrorIs(t, err, ErrDownloadStalled) + assert.Contains(t, err.Error(), "cancelled") +} + +// TestDownloadArchive_StallsBeforeTheFirstByte covers the window the guard is +// running in but the body read is not: DNS, the dial, TLS and the response +// headers across every redirect hop. The stall timeout is shortened because the +// transport built for these tests carries no deadlines of its own, which leaves +// the guard as the only thing watching; production gets to the same verdict +// through the transport instead, and TestDownloadArchive_TransportDeadlineStalls +// pins that path at the shipped stall timeout. Either way a caller who has not +// given up must not be told the download was cancelled. +func TestDownloadArchive_StallsBeforeTheFirstByte(t *testing.T) { + t.Parallel() + + // The handler writes nothing, so net/http sends no response at all and the + // client blocks waiting for the headers. + srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + <-r.Context().Done() + })) + t.Cleanup(srv.Close) + + asset := &otameta.Asset{ + Name: testArchiveName(testStageVersion, archiveExtTarGz), + URL: srv.URL + "/archive", + SHA256: strings.Repeat("00", sha256.Size), + Size: 4096, + } + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + s, err := newStager(opts, testAssetFetcher(t)) + require.NoError(t, err) + s.stallTimeout = 200 * time.Millisecond + + dest := filepath.Join(t.TempDir(), asset.Name) + err = s.downloadArchive(context.Background(), asset, dest) + require.ErrorIs(t, err, ErrDownloadStalled) + assert.Contains(t, err.Error(), "after 0 bytes") + assert.NoFileExists(t, dest) +} + +// TestDownloadArchive_TransportDeadlineStalls is the same dead network as above +// with nothing about the stager weakened: the shipped stall timeout stands, and +// the deadline that fires is the one the transport owns for the response +// headers. A verdict has to come out of that too, or the sentinel would be +// unreachable for the ordinary case of a link that is up and a server that is +// gone. +func TestDownloadArchive_TransportDeadlineStalls(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + <-r.Context().Done() + })) + t.Cleanup(srv.Close) + + transport := &http.Transport{ResponseHeaderTimeout: 200 * time.Millisecond} + t.Cleanup(transport.CloseIdleConnections) + + asset := &otameta.Asset{ + Name: testArchiveName(testStageVersion, archiveExtTarGz), + URL: srv.URL + "/archive", + SHA256: strings.Repeat("00", sha256.Size), + Size: 4096, + } + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + s, err := newStager(opts, assetFetcherFor(transport)) + require.NoError(t, err) + require.Equal(t, downloadStallTimeout, s.stallTimeout) + + dest := filepath.Join(t.TempDir(), asset.Name) + err = s.downloadArchive(context.Background(), asset, dest) + require.ErrorIs(t, err, ErrDownloadStalled) + // Not the release's fault, so not a verdict on it. + require.NotErrorIs(t, err, ErrArchiveRejected) + assert.Contains(t, err.Error(), "after 0 bytes") + assert.NoFileExists(t, dest) +} + +func TestStageRelease_TarGz(t *testing.T) { + t.Parallel() + + assertStagesCleanly(t, archiveExtTarGz) +} + +func TestStageRelease_Zip(t *testing.T) { + t.Parallel() + + assertStagesCleanly(t, archiveExtZip) +} + +func assertStagesCleanly(t *testing.T, ext string) { + t.Helper() + + binary := fakeBinaryBytes(t) + name := testArchiveName(testStageVersion, ext) + asset := servedAsset(t, name, releaseArchive(t, ext, testBinaryName("zaparoo"), binary)) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + + staged, err := stageRelease(context.Background(), opts, testAssetFetcher(t)) + require.NoError(t, err) + require.NotNil(t, staged) + + assert.Equal(t, testStageVersion, staged.Version) + assert.Equal(t, filepath.Join(opts.StagingRoot, testStageVersion), staged.Dir) + assert.Equal(t, filepath.Join(staged.Dir, name), staged.ArchivePath) + assert.Equal(t, filepath.Join(staged.Dir, payloadSubdir, testBinaryName("zaparoo")), staged.BinaryPath) + assert.FileExists(t, staged.ArchivePath) + + got, err := os.ReadFile(staged.BinaryPath) //nolint:gosec // path this package built under t.TempDir + require.NoError(t, err) + assert.Equal(t, binary, got) + + // The licence and readme are in the archive and stay there. + entries, err := os.ReadDir(filepath.Join(staged.Dir, payloadSubdir)) + require.NoError(t, err) + assert.Len(t, entries, 1) + + if runtime.GOOS != "windows" { + info, statErr := os.Stat(staged.BinaryPath) + require.NoError(t, statErr) + assert.NotZero(t, info.Mode().Perm()&0o111, "the staged binary is not executable") + } + + // Nothing outside the staging directory was touched. + assert.NoDirExists(t, filepath.Dir(opts.TargetPath)) +} + +func TestStageRelease_ReplacesAStaleStagingDirectory(t *testing.T) { + t.Parallel() + + ext := archiveExtTarGz + asset := servedAsset(t, testArchiveName(testStageVersion, ext), + releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + + // An attempt that died without cleaning up leaves a directory behind, and + // the archive is created exclusively, so it has to go. + stale := filepath.Join(opts.StagingRoot, testStageVersion) + require.NoError(t, os.MkdirAll(filepath.Join(stale, payloadSubdir), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(stale, testArchiveName(testStageVersion, ext)), + []byte("half a download"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(stale, "junk"), []byte("junk"), 0o600)) + + staged, err := stageRelease(context.Background(), opts, testAssetFetcher(t)) + require.NoError(t, err) + assert.NoFileExists(t, filepath.Join(staged.Dir, "junk")) +} + +// TestStageRelease_PrunesOrphanedStagingDirectories covers the directory no +// later attempt is looking for. The failure path only runs when the process +// lives to reach it, so a power cut mid-download leaves a version directory that +// the next release, computing a different name, would never touch. +func TestStageRelease_PrunesOrphanedStagingDirectories(t *testing.T) { + t.Parallel() + + ext := archiveExtTarGz + asset := servedAsset(t, testArchiveName(testStageVersion, ext), + releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + + orphan := filepath.Join(opts.StagingRoot, "2.9.0") + require.NoError(t, os.MkdirAll(filepath.Join(orphan, payloadSubdir), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(orphan, "half-a-download"), + bytes.Repeat([]byte("x"), 1024), 0o600)) + + staged, err := stageRelease(context.Background(), opts, testAssetFetcher(t)) + require.NoError(t, err) + + assert.NoDirExists(t, orphan, "an orphaned version directory was left on disk") + assert.DirExists(t, staged.Dir, "pruning removed the directory being staged into") +} + +func TestStageRelease_FailureLeavesNothingBehind(t *testing.T) { + t.Parallel() + + ext := archiveExtTarGz + asset := servedAsset(t, testArchiveName(testStageVersion, ext), + releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) + asset.SHA256 = strings.Repeat("cd", sha256.Size) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + + staged, err := stageRelease(context.Background(), opts, testAssetFetcher(t)) + require.ErrorIs(t, err, ErrChecksumMismatch) + assert.Nil(t, staged) + assert.NoDirExists(t, filepath.Join(opts.StagingRoot, testStageVersion)) +} + +func TestStageRelease_ArchiveWithoutTheBinary(t *testing.T) { + t.Parallel() + + ext := archiveExtTarGz + path := filepath.Join(t.TempDir(), "release"+ext) + writeTarGz(t, path, []tarMember{{name: "LICENSE.txt", body: []byte("gpl")}}) + body, err := os.ReadFile(path) //nolint:gosec // test path under t.TempDir + require.NoError(t, err) + + asset := servedAsset(t, testArchiveName(testStageVersion, ext), body) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + + staged, err := stageRelease(context.Background(), opts, testAssetFetcher(t)) + require.ErrorIs(t, err, ErrArchiveRejected) + assert.Nil(t, staged) + assert.NoDirExists(t, filepath.Join(opts.StagingRoot, testStageVersion)) +} + +// TestStageRelease_ProbeFailures is the check that keeps a build which cannot +// run on this device from ever reaching a platform with no supervisor to +// recover it. +func TestStageRelease_ProbeFailures(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + stem string + wantMsg string + binary []byte + probeTimeout time.Duration + }{ + { + name: "exits non-zero", + stem: "zaparoo-fail", + wantMsg: "libz.so.1", + }, + { + name: "answers with the wrong version", + stem: "zaparoo-wrong", + wantMsg: "Zaparoo v0.0.1", + }, + { + name: "never answers", + stem: "zaparoo-hang", + probeTimeout: 300 * time.Millisecond, + wantMsg: "no answer within", + }, + { + name: "is not an executable", + stem: "zaparoo", + binary: bytes.Repeat([]byte("not an executable"), 64), + wantMsg: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + binary := tt.binary + if binary == nil { + binary = fakeBinaryBytes(t) + } + + ext := archiveExtTarGz + member := testBinaryName(tt.stem) + asset := servedAsset(t, testArchiveName(testStageVersion, ext), + releaseArchive(t, ext, member, binary)) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), tt.stem) + + s, err := newStager(opts, testAssetFetcher(t)) + require.NoError(t, err) + if tt.probeTimeout > 0 { + s.probeTimeout = tt.probeTimeout + } + + staged, err := s.run(context.Background()) + require.ErrorIs(t, err, ErrProbeFailed) + assert.Nil(t, staged) + if tt.wantMsg != "" { + assert.Contains(t, err.Error(), tt.wantMsg) + } + // Checked immediately, which the code can be held to: these are the + // subtests that exec a staged binary, and removal is retried for long + // enough to outlast an image file the OS has not finished releasing. + assert.NoDirExists(t, filepath.Join(opts.StagingRoot, testStageVersion)) + }) + } +} + +// TestStageRelease_AcceptsProbeOutputBesideTheVersionLine is the compatibility +// half of the probe. The comparison is made by the build already installed +// against one that did not exist when it shipped, so a future release that prints +// a warning alongside its version must not be condemned as unrunnable for it. +func TestStageRelease_AcceptsProbeOutputBesideTheVersionLine(t *testing.T) { + t.Parallel() + + ext := archiveExtTarGz + asset := servedAsset(t, testArchiveName(testStageVersion, ext), + releaseArchive(t, ext, testBinaryName("zaparoo-chatty"), fakeBinaryBytes(t))) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo-chatty") + + staged, err := stageRelease(context.Background(), opts, testAssetFetcher(t)) + require.NoError(t, err) + require.NotNil(t, staged) + assert.Equal(t, testStageVersion, staged.Version) +} + +// TestStageRelease_ChmodFailureIsLeftToTheProbe covers the filesystem MiSTer and +// MiSTeX install to, which no test host has: /media/fat is vfat or exFAT, it has +// no mode bits, and depending on which driver mounted it chmod either silently +// does nothing or returns an error while the mount's mask has already made the +// file executable. Refusing on the error would reject a release that runs. +// +// Both halves are asserted together, because the first is only safe while the +// second holds: a chmod error on its own does not condemn the release, and the +// probe still does when the binary genuinely cannot execute. +func TestStageRelease_ChmodFailureIsLeftToTheProbe(t *testing.T) { + t.Parallel() + + chmodFailed := errors.New("operation not supported") + + tests := []struct { + wantErr error + chmod func(*testing.T) func(string, os.FileMode) error + name string + wantSuccess bool + }{ + { + name: "mask already granted the exec bit", + chmod: func(t *testing.T) func(string, os.FileMode) error { + t.Helper() + return func(path string, mode os.FileMode) error { + // Standing in for the mount mask: the file ends up executable + // without this call being what did it. On a filesystem that has + // mode bits, doing the chmod for real is how that is reproduced. + //nolint:gosec // G703: staging path under t.TempDir + require.NoError(t, os.Chmod(path, mode)) + return chmodFailed + } + }, + wantSuccess: true, + }, + { + name: "nothing made the binary executable", + chmod: func(t *testing.T) func(string, os.FileMode) error { + t.Helper() + return func(string, os.FileMode) error { return chmodFailed } + }, + wantErr: ErrProbeFailed, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ext := archiveExtTarGz + asset := servedAsset(t, testArchiveName(testStageVersion, ext), + releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) + opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") + s, err := newStager(opts, testAssetFetcher(t)) + require.NoError(t, err) + s.chmod = tt.chmod(t) + + staged, runErr := s.run(context.Background()) + if tt.wantSuccess { + require.NoError(t, runErr) + require.NotNil(t, staged) + assert.Equal(t, testStageVersion, staged.Version) + return + } + require.ErrorIs(t, runErr, tt.wantErr) + assert.Nil(t, staged) + }) + } +} + +// TestProbeBinary_CallerCancellationIsNotAProbeFailure keeps a shutdown from +// condemning a build. A probe that was interrupted never reached a verdict, and +// reporting one would mark a perfectly good release as unrunnable on this device. +func TestProbeBinary_CallerCancellationIsNotAProbeFailure(t *testing.T) { + t.Parallel() + + opts := testStageOptions(t, testRelease("v"+testStageVersion), "zaparoo") + s, err := newStager(opts, unusedFetcher) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { + time.Sleep(150 * time.Millisecond) + cancel() + }() + + err = s.probeBinary(ctx, executableCopy(t, "zaparoo-hang"), testStageVersion) + require.Error(t, err) + require.NotErrorIs(t, err, ErrProbeFailed) + assert.Contains(t, err.Error(), "cancelled") +} + +// executableCopy puts the fake release binary somewhere named stem, which is how +// its behaviour is chosen. +func executableCopy(t *testing.T, stem string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), testBinaryName(stem)) + //nolint:gosec // it has to be executable to be probed at all + require.NoError(t, os.WriteFile(path, fakeBinaryBytes(t), 0o755)) + return path +} + +func TestHasLine(t *testing.T) { + t.Parallel() + + const want = "Zaparoo v2.11.0 (mister)" + + assert.True(t, hasLine(want+"\n", want)) + assert.True(t, hasLine(want, want), "output without a trailing newline") + assert.True(t, hasLine(want+"\r\n", want), "output through a CRLF channel") + assert.True(t, hasLine("warning: something\n"+want+"\n", want)) + assert.True(t, hasLine(want+"\nnote: something\n", want)) + + assert.False(t, hasLine("", want)) + assert.False(t, hasLine("Zaparoo v2.11.0 (linux)\n", want)) + assert.False(t, hasLine("prefix "+want+"\n", want), "a line that merely contains it") + assert.False(t, hasLine(want+" suffix\n", want), "a line that merely starts with it") +} + +func TestStagingRootFor(t *testing.T) { + t.Parallel() + + assert.Empty(t, stagingRootFor("")) + assert.Equal(t, + filepath.Join("data", "updater", stagingSubdir), + stagingRootFor("data")) +} + +func TestClip(t *testing.T) { + t.Parallel() + + assert.Equal(t, "short", clip(" short\n", 32)) + assert.Equal(t, "abc…", clip("abcdef", 3)) +} diff --git a/pkg/service/updater/stall.go b/pkg/service/updater/stall.go new file mode 100644 index 000000000..6d2378ac8 --- /dev/null +++ b/pkg/service/updater/stall.go @@ -0,0 +1,137 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +package updater + +import ( + "context" + "io" + "sync/atomic" + "time" +) + +// stallGuard cancels a transfer that has stopped making progress, without +// putting a ceiling on one that is merely slow. A download to an SD card over a +// congested link can legitimately take minutes, so a wall-clock timeout would +// make those devices unable to update at all; silence is the thing worth +// bounding. A blocked Read cannot time itself out, so a watcher goroutine +// cancels the request context instead. +type stallGuard struct { + cancel context.CancelFunc + done chan struct{} + // start anchors every measurement the guard makes. It carries a monotonic + // reading, so progress is stored and compared as an elapsed duration since + // this instant rather than as a wall-clock instant. That matters on the + // devices this exists for: MiSTer and MiSTeX have no RTC and get stepped by + // NTP shortly after the network comes up, which is the same window an update + // check and download run in. Measured against the wall clock, a forward step + // would abandon a perfectly healthy transfer and a backward step would + // disable stall detection until the step was worked off. + start time.Time + progress atomic.Int64 + fired atomic.Bool + timeout time.Duration +} + +// newStallGuard returns a context to make the request with and the guard +// watching it. The caller must stop the guard once the body is fully read, +// which also releases the context. +func newStallGuard(parent context.Context, timeout time.Duration) (context.Context, *stallGuard) { + ctx, cancel := context.WithCancel(parent) + g := &stallGuard{ + cancel: cancel, + done: make(chan struct{}), + start: time.Now(), + timeout: timeout, + } + g.touch() + go g.watch(ctx) + return ctx, g +} + +// reader wraps a reader so arriving bytes count as progress. +func (g *stallGuard) reader(r io.Reader) io.Reader { + return &progressReader{guard: g, source: r} +} + +// tripped reports whether the guard is what cancelled the context, which is how +// a stall is told apart from the caller giving up. +func (g *stallGuard) tripped() bool { + return g.fired.Load() +} + +// stop ends the watcher and releases the context. Safe to call more than once. +func (g *stallGuard) stop() { + g.cancel() + <-g.done +} + +// touch records that progress happened now, as nanoseconds since the guard +// started rather than as a clock reading. +func (g *stallGuard) touch() { + g.progress.Store(int64(g.since())) +} + +// since is how long the guard has been running, from its monotonic anchor. +func (g *stallGuard) since() time.Duration { + return time.Since(g.start) +} + +func (g *stallGuard) watch(ctx context.Context) { + defer close(g.done) + + // Checking several times per timeout keeps the worst-case detection delay to + // a fraction of it rather than nearly double. The floor is there because + // time.NewTicker panics on a non-positive interval, and nothing in an update + // is worth taking the service down over. + interval := g.timeout / stallChecks + if interval <= 0 { + interval = time.Millisecond + } + ticker := time.NewTicker(interval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + if idle := g.since() - time.Duration(g.progress.Load()); idle >= g.timeout { + g.fired.Store(true) + g.cancel() + return + } + } + } +} + +// progressReader reports every read that produced bytes to its guard. +type progressReader struct { + guard *stallGuard + source io.Reader +} + +func (r *progressReader) Read(p []byte) (int, error) { + n, err := r.source.Read(p) + if n > 0 { + r.guard.touch() + } + //nolint:wrapcheck // a reader wrapper has to pass io.EOF and the source's errors through unchanged + return n, err +} diff --git a/pkg/service/updater/stall_test.go b/pkg/service/updater/stall_test.go new file mode 100644 index 000000000..4c6abed97 --- /dev/null +++ b/pkg/service/updater/stall_test.go @@ -0,0 +1,182 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +package updater + +import ( + "context" + "io" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// trickleReader stands in for a slow but healthy transfer: one byte at a time, +// with a pause between, for far longer in total than the stall timeout. +type trickleReader struct { + interval time.Duration + left int +} + +func (r *trickleReader) Read(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + if r.left == 0 { + return 0, io.EOF + } + time.Sleep(r.interval) + r.left-- + p[0] = 'x' + return 1, nil +} + +// emptyReader returns without producing bytes, which must not count as progress. +type emptyReader struct{ calls int } + +func (r *emptyReader) Read([]byte) (int, error) { + r.calls++ + if r.calls > 1 { + return 0, io.EOF + } + return 0, nil +} + +// TestStallGuard_ProgressKeepsASlowTransferAlive is the property the guard +// exists for, and the one nothing else in the package covers: it bounds silence, +// not duration. This transfer runs four times longer than the timeout while +// never going quiet for it, and has to survive. +// +// It is also the regression test for the tracking itself. The guard records +// progress once when it is constructed, so if reads stopped reporting it this +// would fail while a stall test would not notice. +func TestStallGuard_ProgressKeepsASlowTransferAlive(t *testing.T) { + t.Parallel() + + const ( + timeout = 500 * time.Millisecond + interval = 50 * time.Millisecond + total = 40 + ) + + ctx, guard := newStallGuard(context.Background(), timeout) + defer guard.stop() + + n, err := io.Copy(io.Discard, guard.reader(&trickleReader{interval: interval, left: total})) + require.NoError(t, err) + assert.EqualValues(t, total, n) + assert.False(t, guard.tripped(), "a transfer that never went quiet was reported as stalled") + assert.NoError(t, ctx.Err()) +} + +func TestStallGuard_SilenceTrips(t *testing.T) { + t.Parallel() + + ctx, guard := newStallGuard(context.Background(), 200*time.Millisecond) + defer guard.stop() + + select { + case <-ctx.Done(): + case <-time.After(30 * time.Second): + t.Fatal("the guard never fired on a transfer that made no progress") + } + assert.True(t, guard.tripped(), "the guard cancelled without recording that it was the one that did") +} + +// TestStallGuard_CallerCancelIsNotAStall keeps the two reasons a transfer stops +// distinguishable. Both arrive as a cancelled context, and only the guard's own +// record of firing tells them apart. +func TestStallGuard_CallerCancelIsNotAStall(t *testing.T) { + t.Parallel() + + parent, cancel := context.WithCancel(context.Background()) + ctx, guard := newStallGuard(parent, time.Hour) + defer guard.stop() + + cancel() + <-ctx.Done() + assert.False(t, guard.tripped(), "a caller giving up was recorded as a stall") +} + +func TestStallGuard_StopIsIdempotent(t *testing.T) { + t.Parallel() + + _, guard := newStallGuard(context.Background(), time.Hour) + guard.stop() + guard.stop() +} + +// TestStallGuard_TinyTimeoutDoesNotPanic covers the interval floor. Dividing the +// timeout by the check count can reach zero, and time.NewTicker panics on that. +func TestStallGuard_TinyTimeoutDoesNotPanic(t *testing.T) { + t.Parallel() + + ctx, guard := newStallGuard(context.Background(), time.Nanosecond) + defer guard.stop() + + select { + case <-ctx.Done(): + case <-time.After(30 * time.Second): + t.Fatal("the guard never fired") + } + assert.True(t, guard.tripped()) +} + +func TestProgressReader_CountsBytesAndPassesThrough(t *testing.T) { + t.Parallel() + + _, guard := newStallGuard(context.Background(), time.Hour) + defer guard.stop() + + before := guard.progress.Load() + time.Sleep(time.Millisecond) + + r := guard.reader(strings.NewReader("zaparoo")) + buf := make([]byte, 4) + n, err := r.Read(buf) + require.NoError(t, err) + assert.Equal(t, 4, n) + assert.Equal(t, "zapa", string(buf)) + assert.Greater(t, guard.progress.Load(), before, + "a read that produced bytes did not count as progress") + + rest, err := io.ReadAll(r) + require.NoError(t, err) + assert.Equal(t, "roo", string(rest)) +} + +func TestProgressReader_EmptyReadIsNotProgress(t *testing.T) { + t.Parallel() + + _, guard := newStallGuard(context.Background(), time.Hour) + defer guard.stop() + + time.Sleep(time.Millisecond) + before := guard.progress.Load() + + r := guard.reader(&emptyReader{}) + n, err := r.Read(make([]byte, 4)) + require.NoError(t, err) + assert.Zero(t, n) + assert.Equal(t, before, guard.progress.Load(), + "a read that produced nothing was counted as progress") +} From d7eab934b71fcf58f9862ac6f83ffee06621a2b9 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Tue, 18 Aug 2026 13:07:00 +0800 Subject: [PATCH 3/7] refactor(updater): centralise the archive extensions in otameta extract.go declared its own .tar.gz and .zip constants alongside the ones otameta already used for asset selection. Export ArchiveExtTarGz and ArchiveExtZip from otameta, build its archiveExts list from them, and have extraction reference those instead, so selection and extraction cannot drift on which extensions a release can ship. --- pkg/service/updater/extract.go | 9 ++--- pkg/service/updater/extract_test.go | 53 +++++++++++++------------ pkg/service/updater/otameta/manifest.go | 11 ++++- pkg/service/updater/stage_test.go | 42 ++++++++++---------- 4 files changed, 61 insertions(+), 54 deletions(-) diff --git a/pkg/service/updater/extract.go b/pkg/service/updater/extract.go index 17fa9c979..19609d0cd 100644 --- a/pkg/service/updater/extract.go +++ b/pkg/service/updater/extract.go @@ -40,12 +40,11 @@ import ( "os" "regexp" "strings" + + "github.com/ZaparooProject/zaparoo-core/v2/pkg/service/updater/otameta" ) const ( - archiveExtTarGz = ".tar.gz" - archiveExtZip = ".zip" - // semverPattern matches a version inside an archive member name. semverPattern = `(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)` + `(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?` + @@ -119,9 +118,9 @@ func (s *stager) extractBinary(ctx context.Context, archivePath, ext string, wan } switch ext { - case archiveExtTarGz: + case otameta.ArchiveExtTarGz: return s.extractFromTarGz(ctx, f, destPath) - case archiveExtZip: + case otameta.ArchiveExtZip: return s.extractFromZip(ctx, f, size, destPath) default: return fmt.Errorf("%w: %q is not an archive type this build unpacks", ErrArchiveRejected, ext) diff --git a/pkg/service/updater/extract_test.go b/pkg/service/updater/extract_test.go index 2128e8d9e..dfd8b05f5 100644 --- a/pkg/service/updater/extract_test.go +++ b/pkg/service/updater/extract_test.go @@ -36,6 +36,7 @@ import ( "strings" "testing" + "github.com/ZaparooProject/zaparoo-core/v2/pkg/service/updater/otameta" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -342,7 +343,7 @@ func TestWantsMember_RejectsAnythingWithAPathInIt(t *testing.T) { func TestExtractFromTarGz_TakesOnlyTheBinary(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtTarGz) + h := newExtractHarness(t, otameta.ArchiveExtTarGz) binary := []byte("this stands in for the executable") writeTarGz(t, h.archivePath, []tarMember{ {name: "LICENSE.txt", body: []byte("gpl")}, @@ -350,7 +351,7 @@ func TestExtractFromTarGz_TakesOnlyTheBinary(t *testing.T) { {name: "zaparoo", body: binary, mode: 0o755}, }) - require.NoError(t, h.extract(t, archiveExtTarGz)) + require.NoError(t, h.extract(t, otameta.ArchiveExtTarGz)) got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir require.NoError(t, err) @@ -367,7 +368,7 @@ func TestExtractFromTarGz_TakesOnlyTheBinary(t *testing.T) { func TestExtractFromZip_TakesOnlyTheBinary(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtZip) + h := newExtractHarness(t, otameta.ArchiveExtZip) binary := []byte("this stands in for the executable") writeZip(t, h.archivePath, []zipMember{ {name: "LICENSE.txt", body: []byte("gpl")}, @@ -375,7 +376,7 @@ func TestExtractFromZip_TakesOnlyTheBinary(t *testing.T) { {name: "zaparoo", body: binary, mode: 0o755}, }) - require.NoError(t, h.extract(t, archiveExtZip)) + require.NoError(t, h.extract(t, otameta.ArchiveExtZip)) got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir require.NoError(t, err) @@ -390,13 +391,13 @@ func TestExtractFromZip_TakesOnlyTheBinary(t *testing.T) { func TestExtractFromTarGz_VersionedMemberName(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtTarGz) + h := newExtractHarness(t, otameta.ArchiveExtTarGz) binary := []byte("versioned") writeTarGz(t, h.archivePath, []tarMember{ {name: "zaparoo_2.11.0_linux_amd64", body: binary, mode: 0o755}, }) - require.NoError(t, h.extract(t, archiveExtTarGz)) + require.NoError(t, h.extract(t, otameta.ArchiveExtTarGz)) // It landed under the name this package chose, not the one in the archive. got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir @@ -424,7 +425,7 @@ func TestExtractBinary_UnknownExtension(t *testing.T) { func TestExtractBinary_RejectsBytesChangedOnDisk(t *testing.T) { t.Parallel() - for _, ext := range []string{archiveExtTarGz, archiveExtZip} { + for _, ext := range []string{otameta.ArchiveExtTarGz, otameta.ArchiveExtZip} { t.Run(ext, func(t *testing.T) { t.Parallel() @@ -452,7 +453,7 @@ func TestExtractBinary_RejectsBytesChangedOnDisk(t *testing.T) { func TestExtractBinary_StopsOnCancellation(t *testing.T) { t.Parallel() - for _, ext := range []string{archiveExtTarGz, archiveExtZip} { + for _, ext := range []string{otameta.ArchiveExtTarGz, otameta.ArchiveExtZip} { t.Run(ext, func(t *testing.T) { t.Parallel() @@ -486,13 +487,13 @@ func TestExtractWalk_StopsOnCancellation(t *testing.T) { ext string }{ { - ext: archiveExtTarGz, + ext: otameta.ArchiveExtTarGz, walk: func(h *extractHarness, ctx context.Context, f *os.File, _ int64) error { return h.stager.extractFromTarGz(ctx, f, h.destPath) }, }, { - ext: archiveExtZip, + ext: otameta.ArchiveExtZip, walk: func(h *extractHarness, ctx context.Context, f *os.File, size int64) error { return h.stager.extractFromZip(ctx, f, size, h.destPath) }, @@ -532,7 +533,7 @@ func TestExtractWalk_StopsOnCancellation(t *testing.T) { func TestExtractFromTarGz_InflateBudget(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtTarGz) + h := newExtractHarness(t, otameta.ArchiveExtTarGz) // Compresses to almost nothing on the wire and to well over the budget once // inflated, which is the shape of the attack. writeTarGz(t, h.archivePath, []tarMember{ @@ -541,7 +542,7 @@ func TestExtractFromTarGz_InflateBudget(t *testing.T) { }) h.stager.maxInflatedBytes = 4 << 10 - err := h.extract(t, archiveExtTarGz) + err := h.extract(t, otameta.ArchiveExtTarGz) require.ErrorIs(t, err, ErrArchiveRejected) assert.Contains(t, err.Error(), "bytes of content in it") assert.NoFileExists(t, h.destPath) @@ -553,7 +554,7 @@ func TestExtractFromTarGz_InflateBudget(t *testing.T) { func TestExtractFromZip_NeedsNoInflateBudget(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtZip) + h := newExtractHarness(t, otameta.ArchiveExtZip) binary := []byte("the binary") writeZip(t, h.archivePath, []zipMember{ {name: "filler.bin", body: make([]byte, 64<<10)}, @@ -561,7 +562,7 @@ func TestExtractFromZip_NeedsNoInflateBudget(t *testing.T) { }) h.stager.maxInflatedBytes = 4 << 10 - require.NoError(t, h.extract(t, archiveExtZip)) + require.NoError(t, h.extract(t, otameta.ArchiveExtZip)) got, err := os.ReadFile(h.destPath) //nolint:gosec // test path under t.TempDir require.NoError(t, err) @@ -573,9 +574,9 @@ func writeArchive(t *testing.T, path, ext, memberName string, binary []byte) { t.Helper() switch ext { - case archiveExtTarGz: + case otameta.ArchiveExtTarGz: writeTarGz(t, path, []tarMember{{name: memberName, body: binary, mode: 0o755}}) - case archiveExtZip: + case otameta.ArchiveExtZip: writeZip(t, path, []zipMember{{name: memberName, body: binary, mode: 0o755}}) default: t.Fatalf("unsupported archive extension %q", ext) @@ -714,13 +715,13 @@ func TestExtractFromTarGz_HostileMembers(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtTarGz) + h := newExtractHarness(t, otameta.ArchiveExtTarGz) if tt.limit > 0 { h.stager.maxFileBytes = tt.limit } writeTarGz(t, h.archivePath, tt.members) - err := h.extract(t, archiveExtTarGz) + err := h.extract(t, otameta.ArchiveExtTarGz) require.ErrorIs(t, err, ErrArchiveRejected) assert.Contains(t, err.Error(), tt.wantMsg) }) @@ -835,13 +836,13 @@ func TestExtractFromZip_HostileMembers(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtZip) + h := newExtractHarness(t, otameta.ArchiveExtZip) if tt.limit > 0 { h.stager.maxFileBytes = tt.limit } writeZip(t, h.archivePath, tt.members) - err := h.extract(t, archiveExtZip) + err := h.extract(t, otameta.ArchiveExtZip) require.ErrorIs(t, err, ErrArchiveRejected) assert.Contains(t, err.Error(), tt.wantMsg) }) @@ -854,10 +855,10 @@ func TestExtractFromTarGz_CorruptArchive(t *testing.T) { t.Run("not gzip at all", func(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtTarGz) + h := newExtractHarness(t, otameta.ArchiveExtTarGz) require.NoError(t, os.WriteFile(h.archivePath, []byte("this is not gzip"), 0o600)) - err := h.extract(t, archiveExtTarGz) + err := h.extract(t, otameta.ArchiveExtTarGz) require.ErrorIs(t, err, ErrArchiveRejected) assert.NoFileExists(t, h.destPath) }) @@ -865,7 +866,7 @@ func TestExtractFromTarGz_CorruptArchive(t *testing.T) { t.Run("truncated tar inside valid gzip", func(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtTarGz) + h := newExtractHarness(t, otameta.ArchiveExtTarGz) full := filepath.Join(t.TempDir(), "full.tar.gz") writeTarGz(t, full, []tarMember{ {name: "LICENSE.txt", body: bytes.Repeat([]byte("gpl"), 4096)}, @@ -876,7 +877,7 @@ func TestExtractFromTarGz_CorruptArchive(t *testing.T) { //nolint:gosec // G703: test path under t.TempDir require.NoError(t, os.WriteFile(h.archivePath, body[:len(body)/2], 0o600)) - err = h.extract(t, archiveExtTarGz) + err = h.extract(t, otameta.ArchiveExtTarGz) require.ErrorIs(t, err, ErrArchiveRejected) }) } @@ -884,10 +885,10 @@ func TestExtractFromTarGz_CorruptArchive(t *testing.T) { func TestExtractFromZip_CorruptArchive(t *testing.T) { t.Parallel() - h := newExtractHarness(t, archiveExtZip) + h := newExtractHarness(t, otameta.ArchiveExtZip) require.NoError(t, os.WriteFile(h.archivePath, []byte("this is not a zip"), 0o600)) - err := h.extract(t, archiveExtZip) + err := h.extract(t, otameta.ArchiveExtZip) require.ErrorIs(t, err, ErrArchiveRejected) assert.NoFileExists(t, h.destPath) } diff --git a/pkg/service/updater/otameta/manifest.go b/pkg/service/updater/otameta/manifest.go index 073607698..76096aa8d 100644 --- a/pkg/service/updater/otameta/manifest.go +++ b/pkg/service/updater/otameta/manifest.go @@ -47,8 +47,15 @@ const ( ChannelBeta = "beta" ) -// archiveExts are the extensions release builds package update archives with. -var archiveExts = []string{".tar.gz", ".zip"} +// ArchiveExtTarGz and ArchiveExtZip are the extensions release builds package +// update archives with. They live here because selection decides which one a +// platform gets and extraction has to agree with that decision. +const ( + ArchiveExtTarGz = ".tar.gz" + ArchiveExtZip = ".zip" +) + +var archiveExts = []string{ArchiveExtTarGz, ArchiveExtZip} var ( // ErrNoAsset means no archive in the release is installable here. That is diff --git a/pkg/service/updater/stage_test.go b/pkg/service/updater/stage_test.go index 245fae929..753243ef8 100644 --- a/pkg/service/updater/stage_test.go +++ b/pkg/service/updater/stage_test.go @@ -159,13 +159,13 @@ func releaseArchive(t *testing.T, ext, memberName string, binary []byte) []byte path := filepath.Join(t.TempDir(), "release"+ext) switch ext { - case archiveExtTarGz: + case otameta.ArchiveExtTarGz: writeTarGz(t, path, []tarMember{ {name: "LICENSE.txt", body: []byte("gpl")}, {name: "README.txt", body: []byte("readme")}, {name: memberName, body: binary, mode: 0o755}, }) - case archiveExtZip: + case otameta.ArchiveExtZip: writeZip(t, path, []zipMember{ {name: "LICENSE.txt", body: []byte("gpl")}, {name: "README.txt", body: []byte("readme")}, @@ -355,7 +355,7 @@ func TestSelectArchive(t *testing.T) { t.Parallel() asset := func(version string) *otameta.Asset { - return &otameta.Asset{Name: testArchiveName(version, archiveExtTarGz), Size: 1024} + return &otameta.Asset{Name: testArchiveName(version, otameta.ArchiveExtTarGz), Size: 1024} } tests := []struct { @@ -483,7 +483,7 @@ func TestSelectArchive(t *testing.T) { name: "no archive for this platform", build: func() *otameta.Release { return testRelease("v2.11.0", &otameta.Asset{ - Name: otameta.ArchiveBaseName("windows", "amd64", "2.11.0") + archiveExtZip, + Name: otameta.ArchiveBaseName("windows", "amd64", "2.11.0") + otameta.ArchiveExtZip, }) }, current: "2.10.1", @@ -505,7 +505,7 @@ func TestSelectArchive(t *testing.T) { require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, tt.wantVersion, version) - assert.Equal(t, testArchiveName(tt.wantVersion, archiveExtTarGz), got.Name) + assert.Equal(t, testArchiveName(tt.wantVersion, otameta.ArchiveExtTarGz), got.Name) return } @@ -525,7 +525,7 @@ func TestSelectArchive_ArmDoesNotPickUpArm64(t *testing.T) { t.Parallel() rel := testRelease("v2.11.0", &otameta.Asset{ - Name: otameta.ArchiveBaseName("mister", "arm64", "2.11.0") + archiveExtZip, + Name: otameta.ArchiveBaseName("mister", "arm64", "2.11.0") + otameta.ArchiveExtZip, }) opts := testStageOptions(t, rel, "zaparoo.sh") opts.PlatformID = "mister" @@ -607,7 +607,7 @@ func TestDownloadArchive(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - asset := servedAsset(t, testArchiveName(testStageVersion, archiveExtTarGz), body) + asset := servedAsset(t, testArchiveName(testStageVersion, otameta.ArchiveExtTarGz), body) tt.mutate(asset) opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") @@ -645,7 +645,7 @@ func TestDownloadArchive_ServerError(t *testing.T) { t.Cleanup(srv.Close) asset := &otameta.Asset{ - Name: testArchiveName(testStageVersion, archiveExtTarGz), + Name: testArchiveName(testStageVersion, otameta.ArchiveExtTarGz), URL: srv.URL + "/missing", SHA256: strings.Repeat("00", sha256.Size), Size: 1024, @@ -679,7 +679,7 @@ func TestDownloadArchive_Stalls(t *testing.T) { sum := sha256.Sum256(body) asset := &otameta.Asset{ - Name: testArchiveName(testStageVersion, archiveExtTarGz), + Name: testArchiveName(testStageVersion, otameta.ArchiveExtTarGz), URL: srv.URL + "/archive", SHA256: hex.EncodeToString(sum[:]), Size: int64(len(body)), @@ -712,7 +712,7 @@ func TestDownloadArchive_CallerCancels(t *testing.T) { sum := sha256.Sum256(body) asset := &otameta.Asset{ - Name: testArchiveName(testStageVersion, archiveExtTarGz), + Name: testArchiveName(testStageVersion, otameta.ArchiveExtTarGz), URL: srv.URL + "/archive", SHA256: hex.EncodeToString(sum[:]), Size: int64(len(body)), @@ -756,7 +756,7 @@ func TestDownloadArchive_StallsBeforeTheFirstByte(t *testing.T) { t.Cleanup(srv.Close) asset := &otameta.Asset{ - Name: testArchiveName(testStageVersion, archiveExtTarGz), + Name: testArchiveName(testStageVersion, otameta.ArchiveExtTarGz), URL: srv.URL + "/archive", SHA256: strings.Repeat("00", sha256.Size), Size: 4096, @@ -791,7 +791,7 @@ func TestDownloadArchive_TransportDeadlineStalls(t *testing.T) { t.Cleanup(transport.CloseIdleConnections) asset := &otameta.Asset{ - Name: testArchiveName(testStageVersion, archiveExtTarGz), + Name: testArchiveName(testStageVersion, otameta.ArchiveExtTarGz), URL: srv.URL + "/archive", SHA256: strings.Repeat("00", sha256.Size), Size: 4096, @@ -813,13 +813,13 @@ func TestDownloadArchive_TransportDeadlineStalls(t *testing.T) { func TestStageRelease_TarGz(t *testing.T) { t.Parallel() - assertStagesCleanly(t, archiveExtTarGz) + assertStagesCleanly(t, otameta.ArchiveExtTarGz) } func TestStageRelease_Zip(t *testing.T) { t.Parallel() - assertStagesCleanly(t, archiveExtZip) + assertStagesCleanly(t, otameta.ArchiveExtZip) } func assertStagesCleanly(t *testing.T, ext string) { @@ -862,7 +862,7 @@ func assertStagesCleanly(t *testing.T, ext string) { func TestStageRelease_ReplacesAStaleStagingDirectory(t *testing.T) { t.Parallel() - ext := archiveExtTarGz + ext := otameta.ArchiveExtTarGz asset := servedAsset(t, testArchiveName(testStageVersion, ext), releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") @@ -887,7 +887,7 @@ func TestStageRelease_ReplacesAStaleStagingDirectory(t *testing.T) { func TestStageRelease_PrunesOrphanedStagingDirectories(t *testing.T) { t.Parallel() - ext := archiveExtTarGz + ext := otameta.ArchiveExtTarGz asset := servedAsset(t, testArchiveName(testStageVersion, ext), releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") @@ -907,7 +907,7 @@ func TestStageRelease_PrunesOrphanedStagingDirectories(t *testing.T) { func TestStageRelease_FailureLeavesNothingBehind(t *testing.T) { t.Parallel() - ext := archiveExtTarGz + ext := otameta.ArchiveExtTarGz asset := servedAsset(t, testArchiveName(testStageVersion, ext), releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) asset.SHA256 = strings.Repeat("cd", sha256.Size) @@ -922,7 +922,7 @@ func TestStageRelease_FailureLeavesNothingBehind(t *testing.T) { func TestStageRelease_ArchiveWithoutTheBinary(t *testing.T) { t.Parallel() - ext := archiveExtTarGz + ext := otameta.ArchiveExtTarGz path := filepath.Join(t.TempDir(), "release"+ext) writeTarGz(t, path, []tarMember{{name: "LICENSE.txt", body: []byte("gpl")}}) body, err := os.ReadFile(path) //nolint:gosec // test path under t.TempDir @@ -983,7 +983,7 @@ func TestStageRelease_ProbeFailures(t *testing.T) { binary = fakeBinaryBytes(t) } - ext := archiveExtTarGz + ext := otameta.ArchiveExtTarGz member := testBinaryName(tt.stem) asset := servedAsset(t, testArchiveName(testStageVersion, ext), releaseArchive(t, ext, member, binary)) @@ -1016,7 +1016,7 @@ func TestStageRelease_ProbeFailures(t *testing.T) { func TestStageRelease_AcceptsProbeOutputBesideTheVersionLine(t *testing.T) { t.Parallel() - ext := archiveExtTarGz + ext := otameta.ArchiveExtTarGz asset := servedAsset(t, testArchiveName(testStageVersion, ext), releaseArchive(t, ext, testBinaryName("zaparoo-chatty"), fakeBinaryBytes(t))) opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo-chatty") @@ -1076,7 +1076,7 @@ func TestStageRelease_ChmodFailureIsLeftToTheProbe(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - ext := archiveExtTarGz + ext := otameta.ArchiveExtTarGz asset := servedAsset(t, testArchiveName(testStageVersion, ext), releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) opts := testStageOptions(t, testRelease("v"+testStageVersion, asset), "zaparoo") From 979a4cd542030007c87d431c90fce71688ffcd16 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Tue, 18 Aug 2026 13:07:09 +0800 Subject: [PATCH 4/7] test(updater): skip the exec-bit chmod case on windows The "nothing made the binary executable" case asserts the probe catches a staged binary that no chmod ever made runnable. Windows has no permission bit to withhold, so the file runs either way and the probe has nothing to catch. Guard the case the same way the exec-bit assertion in assertStagesCleanly already is; the chmod failure and ErrProbeFailed assertions are unchanged everywhere else. --- pkg/service/updater/stage_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/service/updater/stage_test.go b/pkg/service/updater/stage_test.go index 753243ef8..70bcf860c 100644 --- a/pkg/service/updater/stage_test.go +++ b/pkg/service/updater/stage_test.go @@ -1046,6 +1046,7 @@ func TestStageRelease_ChmodFailureIsLeftToTheProbe(t *testing.T) { chmod func(*testing.T) func(string, os.FileMode) error name string wantSuccess bool + needsModes bool }{ { name: "mask already granted the exec bit", @@ -1068,7 +1069,8 @@ func TestStageRelease_ChmodFailureIsLeftToTheProbe(t *testing.T) { t.Helper() return func(string, os.FileMode) error { return chmodFailed } }, - wantErr: ErrProbeFailed, + wantErr: ErrProbeFailed, + needsModes: true, }, } @@ -1076,6 +1078,12 @@ func TestStageRelease_ChmodFailureIsLeftToTheProbe(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() + if tt.needsModes && runtime.GOOS == "windows" { + // There is no exec bit to withhold: the staged file runs whether + // the chmod worked or not, so the probe has nothing to catch. + t.Skip("windows has no permission bit for a failed chmod to leave unset") + } + ext := otameta.ArchiveExtTarGz asset := servedAsset(t, testArchiveName(testStageVersion, ext), releaseArchive(t, ext, testBinaryName("zaparoo"), fakeBinaryBytes(t))) From a039ff5fdfcbc3792240fdf7252b54bc539e0a62 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Tue, 18 Aug 2026 13:07:18 +0800 Subject: [PATCH 5/7] fix(updater): bound the output the version probe keeps probeBinary drained the staged binary's stdout and stderr into unbounded strings.Builders. The binary being probed arrived over the network moments earlier, and one that fails by printing without stopping would be held whole in memory on a device with a few hundred megabytes of it. Keep the first 8 KiB of each stream and drop the rest, still reporting every write as consumed so the process drains instead of blocking on the pipe. Only the first line is read, so nothing the error message uses is lost. --- pkg/service/updater/stage.go | 30 +++++++++++++++++++++++++++++- pkg/service/updater/stage_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/pkg/service/updater/stage.go b/pkg/service/updater/stage.go index 79660ea27..f7ebe44f9 100644 --- a/pkg/service/updater/stage.go +++ b/pkg/service/updater/stage.go @@ -87,6 +87,14 @@ const ( // update rather than fail it. probeWaitDelay = 2 * time.Second + // probeOutputLimit caps how much of the staged binary's output the probe + // keeps. The probe runs a binary that arrived over the network moments ago, + // and one that fails by printing without stopping would otherwise be held + // whole in memory on a device that has a few hundred megabytes of it. Only + // the first line matters here, so keeping a few kilobytes loses nothing the + // error message would have used. + probeOutputLimit = 8 << 10 + // stagingRemoveAttempts and stagingRemoveDelay bound how long removing a // staging directory is retried. A staging directory holds a binary this // process may have just finished executing for the probe, and Windows can keep @@ -188,6 +196,26 @@ type StagedUpdate struct { // body; tests serve archives from a local server. type assetFetcher func(ctx context.Context, target string) (io.ReadCloser, error) +// cappedBuilder keeps the first probeOutputLimit bytes written to it and +// discards the rest. It reports every write as fully consumed, so the process +// on the other end drains normally instead of blocking on a pipe nobody is +// reading. +type cappedBuilder struct { + buf strings.Builder +} + +func (b *cappedBuilder) Write(p []byte) (int, error) { + if room := probeOutputLimit - b.buf.Len(); room > 0 { + // strings.Builder.Write never fails. + _, _ = b.buf.Write(p[:min(room, len(p))]) + } + return len(p), nil +} + +func (b *cappedBuilder) String() string { + return b.buf.String() +} + // stager holds the resolved settings for one staging attempt. The limits and // timeouts are fields rather than constants read at the point of use so tests // can drive the guards without a 256 MB fixture or a 90 second wait. @@ -618,7 +646,7 @@ func (s *stager) probeBinary(ctx context.Context, binaryPath, version string) er //nolint:gosec // the path is a file this process just created inside its own staging directory cmd := exec.CommandContext(probeCtx, binaryPath, "-"+config.VersionFlagName) - var stdout, stderr strings.Builder + var stdout, stderr cappedBuilder cmd.Stdout = &stdout cmd.Stderr = &stderr // Killing the process is not enough to unblock Wait if it left a child diff --git a/pkg/service/updater/stage_test.go b/pkg/service/updater/stage_test.go index 70bcf860c..43d9b47b7 100644 --- a/pkg/service/updater/stage_test.go +++ b/pkg/service/updater/stage_test.go @@ -1171,3 +1171,29 @@ func TestClip(t *testing.T) { assert.Equal(t, "short", clip(" short\n", 32)) assert.Equal(t, "abc…", clip("abcdef", 3)) } + +// TestCappedBuilder_KeepsThePrefixAndDrainsTheRest covers the probe's output +// handling: a staged binary that fails by printing without stopping must not be +// able to grow the updater's memory, and must not block on a pipe nobody reads +// either, so every write is reported as consumed whatever happened to it. +func TestCappedBuilder_KeepsThePrefixAndDrainsTheRest(t *testing.T) { + t.Parallel() + + var b cappedBuilder + first := strings.Repeat("a", probeOutputLimit-1) + n, err := b.Write([]byte(first)) + require.NoError(t, err) + assert.Equal(t, len(first), n) + + n, err = b.Write([]byte("bcde")) + require.NoError(t, err) + assert.Equal(t, 4, n, "a partially kept write still has to report every byte consumed") + + n, err = b.Write([]byte(strings.Repeat("f", 1<<20))) + require.NoError(t, err) + assert.Equal(t, 1<<20, n, "a write past the cap still has to report every byte consumed") + + got := b.String() + assert.Len(t, got, probeOutputLimit) + assert.Equal(t, first+"b", got) +} From 6618d91942fa6dad9493c2ac7c1df316ace288dc Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Tue, 18 Aug 2026 13:07:31 +0800 Subject: [PATCH 6/7] test(updater): give the stall guard progress test a real interval The test slept 1 ms between reading the guard's progress counter and the read that should advance it, which leaves the assertion resting on the platform's timer resolution. Sleep 20 ms so since() has moved on everywhere. The read and progress assertions are unchanged. --- pkg/service/updater/stall_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/service/updater/stall_test.go b/pkg/service/updater/stall_test.go index 4c6abed97..21147a7a8 100644 --- a/pkg/service/updater/stall_test.go +++ b/pkg/service/updater/stall_test.go @@ -148,7 +148,10 @@ func TestProgressReader_CountsBytesAndPassesThrough(t *testing.T) { defer guard.stop() before := guard.progress.Load() - time.Sleep(time.Millisecond) + // Long enough that since() has moved on even where the clock is coarse, so + // the assertion below is about the read counting as progress rather than + // about the platform's timer resolution. + time.Sleep(20 * time.Millisecond) r := guard.reader(strings.NewReader("zaparoo")) buf := make([]byte, 4) From cd66a6a3abd6db8d010ab2721b43a5aa5746c752 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Tue, 18 Aug 2026 13:07:41 +0800 Subject: [PATCH 7/7] fix(updater): let cancellation end the staging cleanup retries removeStagingDir retries a failed removal up to twenty times with a 100 ms sleep between attempts, which is how a Windows sharing violation on a binary the probe just executed gets cleared. Nothing interrupted it, so a shutdown during staging could wait two seconds per directory while pruneStagingRoot worked through the orphans. Thread the context from run through pruneStagingRoot into removeStagingDir and end the loop when it is cancelled. The first attempt still always runs, so the ordinary path never consults the context, and a directory abandoned mid-retry is collected by the next attempt's sweep. --- pkg/service/updater/stage.go | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/pkg/service/updater/stage.go b/pkg/service/updater/stage.go index f7ebe44f9..e65fa7567 100644 --- a/pkg/service/updater/stage.go +++ b/pkg/service/updater/stage.go @@ -335,10 +335,10 @@ func (s *stager) run(ctx context.Context) (*StagedUpdate, error) { // A previous attempt that died without cleaning up would otherwise collide // with this one's exclusive file creation. - if rmErr := removeStagingDir(dir); rmErr != nil { + if rmErr := removeStagingDir(ctx, dir); rmErr != nil { return nil, fmt.Errorf("clearing previous update staging directory: %w", rmErr) } - pruneStagingRoot(s.stagingRoot, version) + pruneStagingRoot(ctx, s.stagingRoot, version) //nolint:gosec // G703: the version is asserted above to be a single path element if mkErr := os.MkdirAll(dir, stateDirPerm); mkErr != nil { return nil, fmt.Errorf("creating update staging directory: %w", mkErr) @@ -348,7 +348,7 @@ func (s *stager) run(ctx context.Context) (*StagedUpdate, error) { if err != nil { // Nothing outside this directory has been written, so discarding it // leaves no trace of the attempt. - if rmErr := removeStagingDir(dir); rmErr != nil { + if rmErr := removeStagingDir(ctx, dir); rmErr != nil { log.Warn().Err(rmErr).Str("dir", dir).Msg("could not remove failed update staging directory") } return nil, err @@ -374,7 +374,7 @@ func (s *stager) run(ctx context.Context) (*StagedUpdate, error) { // // Failures are logged and not returned. Being unable to tidy up is not a reason // to refuse an update. -func pruneStagingRoot(root, keep string) { +func pruneStagingRoot(ctx context.Context, root, keep string) { entries, err := os.ReadDir(root) if err != nil { if !errors.Is(err, os.ErrNotExist) { @@ -388,7 +388,7 @@ func pruneStagingRoot(root, keep string) { continue } stale := filepath.Join(root, entry.Name()) - if rmErr := removeStagingDir(stale); rmErr != nil { + if rmErr := removeStagingDir(ctx, stale); rmErr != nil { log.Warn().Err(rmErr).Str("dir", stale).Msg("could not remove an orphaned update staging directory") continue } @@ -400,12 +400,19 @@ func pruneStagingRoot(root, keep string) { // // See stagingRemoveAttempts for why one attempt is not enough. Sleeping only // happens when a removal has actually failed, so the ordinary path is a single -// call. -func removeStagingDir(dir string) error { +// call and never consults the context. Once it is retrying, a cancelled context +// ends it: two seconds of sharing-violation retries is not worth holding up a +// shutdown for, and the directory left behind is collected by the next +// attempt's sweep. +func removeStagingDir(ctx context.Context, dir string) error { var err error for attempt := range stagingRemoveAttempts { if attempt > 0 { - time.Sleep(stagingRemoveDelay) + select { + case <-ctx.Done(): + return fmt.Errorf("removing update staging directory %q: %w", dir, ctx.Err()) + case <-time.After(stagingRemoveDelay): + } } //nolint:gosec // G703: callers pass a path this package built under its own staging root if err = os.RemoveAll(dir); err == nil {