From 9d75a29c3aabbbe9a6c3880e29e830829bb5f17d Mon Sep 17 00:00:00 2001 From: indaco Date: Sun, 26 Jul 2026 19:05:46 +0200 Subject: [PATCH 1/2] fix(install): resolve dependency-scoped bottle placeholders Relocation substituted only the prefix-derived tokens, so a bottle whose wrapper references a path inside another keg shipped that token literally and was unusable at launch. That value cannot be derived from the prefix, so the install path resolves it from the formula's declared dependencies and passes one extra replacement down to the keg relocation. The extra replacement is applied last: text replacements run as sequential passes, and the path it substitutes must not be rewritten again by the prefix rules. Bumps the relocation logic version so cached kegs holding the literal token are re-relocated rather than restored still broken. --- ...ndency-scoped-placeholder-unsubstituted.sh | 126 ++++++++++++ .../relocated-store-logic-version.pin | 4 +- src/cli/install/download.zig | 14 ++ src/cli/migrate/keg.zig | 16 +- src/cli/rollback.zig | 59 +++++- src/core/cellar.zig | 36 +++- src/core/formula.zig | 192 ++++++++++++++++++ src/core/relocated_store.zig | 6 +- tests/cellar_test.zig | 126 +++++++++++- tests/install_warm_path_test.zig | 4 + 10 files changed, 567 insertions(+), 16 deletions(-) create mode 100755 scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh diff --git a/scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh b/scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh new file mode 100755 index 00000000..9782bef5 --- /dev/null +++ b/scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh @@ -0,0 +1,126 @@ +#!/usr/bin/env bash +# Regression: keg relocation substituted only the tokens it could derive from +# the prefix. A bottle whose wrapper script references a path inside ANOTHER +# keg carries a placeholder whose value is not a function of the prefix, so the +# token shipped literally into `/bin/`. The wrapper then resolved to a +# path that does not exist and failed at launch, behind an install that +# reported success and a `doctor` run that reported the prefix healthy. +# +# Two halves have to stay fixed, and each fails differently: +# 1. relocation must accept and apply a caller-resolved replacement, and the +# install path must resolve it from the formula's declared dependencies; +# 2. RELOC_LOGIC_VERSION must have moved past 2, or `store-relocated/v2/` +# still serves kegs snapshotted with the literal token and the fix never +# reaches anyone who already installed an affected bottle. +# +# Behavioural guards live in colocated `test {}` blocks; this script asserts +# the wiring is present statically, then runs the two test binaries and judges +# those guards' lines. Exits 0 when the bug is absent, non-zero (naming the +# failing assertion) when present. No network; no temp state; well under 30s +# once the test binaries are built. + +set -euo pipefail + +ROOT=$(cd "$(dirname "$0")/../.." && pwd) + +# 1. The resolver must exist as a table, not a hardcoded lookup: the token set +# is upstream's, so new entries have to be a data change. +if ! grep -q 'dependency_placeholders' "$ROOT/src/core/formula.zig"; then + echo "FAIL: formula.zig no longer declares the dependency placeholder table" >&2 + exit 1 +fi +if ! grep -q 'pub fn dependencyPlaceholder' "$ROOT/src/core/formula.zig"; then + echo "FAIL: formula.dependencyPlaceholder is gone — nothing resolves the token" >&2 + exit 1 +fi + +# 2. Relocation must actually apply the caller's replacement. Without this the +# resolver above is dead code and the token still ships literally. +if ! grep -q 'extra_replacement' "$ROOT/src/core/cellar.zig"; then + echo "FAIL: relocateKegTree no longer applies a caller-resolved replacement" >&2 + exit 1 +fi + +# 3. Every path that materialises a bottle from the API must resolve the token; +# the formula is in scope at each. `migrate` shares the bottle path with +# `install`, and reaching for the short `materialize()` wrapper there is the +# easy way to silently reintroduce the bug for migrated kegs. +for site in src/cli/install/download.zig src/cli/migrate/keg.zig; do + if ! grep -q 'dependencyPlaceholder' "$ROOT/$site"; then + echo "FAIL: $site no longer resolves the placeholder from the formula" >&2 + exit 1 + fi +done + +# 4. Cached kegs relocated before the fix hold the literal token. Serving one +# back re-breaks a fixed binary, so the cache version must be past 2. +version=$(grep -oE 'RELOC_LOGIC_VERSION: u32 = [0-9]+' "$ROOT/src/core/relocated_store.zig" | + grep -oE '[0-9]+$' || true) +if [[ -z "$version" ]]; then + echo "FAIL: could not read RELOC_LOGIC_VERSION from relocated_store.zig" >&2 + exit 1 +fi +if ((version < 3)); then + echo "FAIL: RELOC_LOGIC_VERSION is $version; kegs cached under v2 still hold the literal token" >&2 + exit 1 +fi + +# 5. Behavioural guards. If either test block is deleted the runs below would +# silently pass, so assert they are present first. +RESOLVE_TEST="dependencyPlaceholder resolves a token against its declared dependency" +PINNED_TEST="dependencyPlaceholder keeps the version suffix of a pinned dependency" +APPLY_TEST="relocation substitutes the caller-resolved dependency placeholder" +WITHHOLD_TEST="relocation leaves an unresolved dependency placeholder in place" + +if ! grep -Rqs -- "$RESOLVE_TEST" "$ROOT/src/core/formula.zig"; then + echo "FAIL: resolver guard test missing from formula.zig" >&2 + exit 1 +fi +if ! grep -Rqs -- "$PINNED_TEST" "$ROOT/src/core/formula.zig"; then + echo "FAIL: pinned-dependency guard test missing from formula.zig" >&2 + exit 1 +fi +if ! grep -Rqs -- "$APPLY_TEST" "$ROOT/tests/cellar_test.zig"; then + echo "FAIL: relocation guard test missing from cellar_test.zig" >&2 + exit 1 +fi +if ! grep -Rqs -- "$WITHHOLD_TEST" "$ROOT/tests/cellar_test.zig"; then + echo "FAIL: withhold guard test missing from cellar_test.zig" >&2 + exit 1 +fi + +# Rebuild the test binaries so the run reflects the working tree. +if ! (cd "$ROOT" && zig build test-bin >/dev/null 2>&1); then + echo "FAIL: could not build the test binaries (zig build test-bin)" >&2 + exit 1 +fi + +# The runners have no per-test filter, so run the suite ONCE and judge only the +# guard lines: a pass ends in "OK". +# +# Deliberate scope limit: the two resolver guards are inline `test {}` blocks in +# src/, so they only run inside `lib_tests` — ~2300 tests and ~38s, which alone +# blows this suite's 30s budget. They are asserted present above (deleting one +# fails this script) and executed by `just test`; what runs here is the +# end-to-end half, which is where the bug actually lived. +check() { + local bin="$1" + shift + local out line name + out=$("$ROOT/zig-out/test-bin/$bin" 2>&1 || true) + for name in "$@"; do + line=$(printf '%s\n' "$out" | grep -F -- "$name" || true) + if [[ -z "$line" ]]; then + echo "FAIL: guard test did not run in $bin: $name" >&2 + exit 1 + fi + if [[ "$line" != *OK ]]; then + echo "FAIL: $name: $line" >&2 + exit 1 + fi + done +} + +check cellar_test "$APPLY_TEST" "$WITHHOLD_TEST" + +echo "OK: dependency-scoped placeholders resolved and applied; cache version past v2" diff --git a/scripts/regressions/relocated-store-logic-version.pin b/scripts/regressions/relocated-store-logic-version.pin index b853807e..48a32c3a 100644 --- a/scripts/regressions/relocated-store-logic-version.pin +++ b/scripts/regressions/relocated-store-logic-version.pin @@ -1,2 +1,2 @@ -version=2 -digest=a49588f0cec4692dcb00209f3306d40df1b3faec3968428e73aeb92eecf95290 +version=3 +digest=f1ed07e25bc179c84b72e73b3a8903e6afd9ba3745f5d4a9a44773feacc2f8f9 diff --git a/src/cli/install/download.zig b/src/cli/install/download.zig index 370840d2..2071a904 100644 --- a/src/cli/install/download.zig +++ b/src/cli/install/download.zig @@ -9,6 +9,7 @@ const bottle_mod = @import("../../core/bottle.zig"); const cellar_mod = @import("../../core/cellar.zig"); const deps_mod = @import("../../core/deps.zig"); const formula_mod = @import("../../core/formula.zig"); +const patch_mod = @import("../../core/patch.zig"); const signals = @import("../../core/signals.zig"); const store_mod = @import("../../core/store.zig"); const sqlite = @import("../../db/sqlite.zig"); @@ -577,6 +578,18 @@ pub fn installKegFromBottle( }; } + // Some bottles carry placeholder tokens whose value is a path inside a + // *declared dependency's* keg rather than the prefix. Only here is the + // formula in scope to resolve them, so the value is passed down. + var placeholder_buf: [512]u8 = undefined; + const placeholder = formula_mod.dependencyPlaceholder( + &placeholder_buf, + target_prefix, + formula.dependencies, + ); + const extra_replacement: ?patch_mod.Replacement = + if (placeholder) |p| .{ .old = p.token, .new = p.value } else null; + const keg = cellar_mod.materializeWithCellar( ctx.io, allocator, @@ -585,6 +598,7 @@ pub fn installKegFromBottle( formula.name, formula.pkg_version, bottle.cellar, + extra_replacement, ) catch |err| { if (deps.cellar_diag) |out| out.* = err; return InstallError.CellarFailed; diff --git a/src/cli/migrate/keg.zig b/src/cli/migrate/keg.zig index ccd15312..014809a0 100644 --- a/src/cli/migrate/keg.zig +++ b/src/cli/migrate/keg.zig @@ -154,13 +154,27 @@ pub fn migrateKeg( // before materialise (the expensive step stays parallel, lock-free). incrementRefLocked(ctx.io, deps.store, deps.db_mu, bottle.sha256, keg_name); - const keg = cellar_mod.materialize( + // Same bottle path as a fresh install, so it needs the same + // dependency-scoped placeholder resolution. + var placeholder_buf: [512]u8 = undefined; + const placeholder = formula_mod.dependencyPlaceholder( + &placeholder_buf, + deps.prefix, + formula.dependencies, + ); + + const keg = cellar_mod.materializeWithCellar( ctx.io, allocator, deps.prefix, bottle.sha256, formula.name, formula.pkg_version, + // Unchanged from the `materialize` wrapper this replaced: migrate has + // always relocated these kegs unconditionally, and the cellar type is + // a separate question from the placeholder. + "", + if (placeholder) |p| .{ .old = p.token, .new = p.value } else null, ) catch { output.err(" {s}: failed to materialize", .{keg_name}); output.emitNdjsonEvent(.materialized, keg_name, "failed"); diff --git a/src/cli/rollback.zig b/src/cli/rollback.zig index fb5436ef..9d7b157b 100644 --- a/src/cli/rollback.zig +++ b/src/cli/rollback.zig @@ -185,8 +185,31 @@ pub fn execute(ctx: *const AppCtx, allocator: std.mem.Allocator, args: []const [ output.warn("Could not remove cellar entry for {s} {s}", .{ name, current_pkg_version }); }; + // No parsed formula here, and the DB rows describe the version being + // rolled away from — so read the target's own shipped formula source. + var ph_buf: [512]u8 = undefined; + const placeholder = storeKegPlaceholder( + ctx.io, + allocator, + &ph_buf, + prefix, + target.sha256, + name, + target.pkg_version, + ); + // Materialize the old version from store - const keg = cellar.materialize(ctx.io, allocator, prefix, target.sha256, name, target.pkg_version) catch { + const keg = cellar.materializeWithCellar( + ctx.io, + allocator, + prefix, + target.sha256, + name, + target.pkg_version, + // Unchanged from the `materialize` wrapper this replaced. + "", + if (placeholder) |p| .{ .old = p.token, .new = p.value } else null, + ) catch { output.err("Failed to materialize {s} {s} from store", .{ name, target.pkg_version }); return error.Aborted; }; @@ -863,6 +886,40 @@ test "writeEntriesJsonArray emits [] when empty" { try testing.expectEqualStrings("[]", aw.written()); } +/// Resolve a store keg's placeholder from the formula source its bottle ships +/// at `.brew/.rb`. Best-effort: null leaves the token in place, which +/// doctor's placeholder check then reports. +fn storeKegPlaceholder( + io: std.Io, + allocator: std.mem.Allocator, + buf: []u8, + prefix: []const u8, + sha256: []const u8, + name: []const u8, + pkg_version: []const u8, +) ?formula_mod.Placeholder { + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const rb_path = std.fmt.bufPrint( + &path_buf, + "{s}/store/{s}/{s}/{s}/.brew/{s}.rb", + .{ prefix, sha256, name, pkg_version, name }, + ) catch return null; + + const file = std.Io.Dir.openFileAbsolute(io, rb_path, .{}) catch return null; + defer file.close(io); + + const stat = file.stat(io) catch return null; + if (stat.size == 0 or stat.size > 1024 * 1024) return null; + const src = allocator.alloc(u8, stat.size) catch return null; + defer allocator.free(src); + const read = file.readPositionalAll(io, src, 0) catch return null; + if (read < src.len) return null; + + var dep_buf: [64][]const u8 = undefined; + const deps = formula_mod.declaredDependencies(&dep_buf, src); + return formula_mod.dependencyPlaceholder(buf, prefix, deps); +} + /// Seed a `/store////INSTALL_RECEIPT.json` /// tree. `delay_ms_after` lets the caller force monotonically increasing /// mtimes across calls without depending on filesystem timer resolution diff --git a/src/core/cellar.zig b/src/core/cellar.zig index bb5f9df5..aa698e45 100644 --- a/src/core/cellar.zig +++ b/src/core/cellar.zig @@ -71,13 +71,16 @@ pub fn materialize( name: []const u8, version: []const u8, ) CellarError!Keg { - return materializeWithCellar(io, allocator, prefix, store_sha256, name, version, ""); + return materializeWithCellar(io, allocator, prefix, store_sha256, name, version, "", null); } /// Materialize with an explicit cellar type from the bottle metadata. /// When cellar_type is ":any" or ":any_skip_relocation", Mach-O binary /// patching is skipped (relocatable bottle). Text placeholder substitution /// (@@HOMEBREW_PREFIX@@, @@HOMEBREW_CELLAR@@) always runs. +/// +/// `extra_replacement` carries a substitution the caller resolved from the +/// formula (see `formula.dependencyPlaceholder`); null for most bottles. pub fn materializeWithCellar( io: std.Io, allocator: std.mem.Allocator, @@ -86,6 +89,7 @@ pub fn materializeWithCellar( name: []const u8, version: []const u8, cellar_type: []const u8, + extra_replacement: ?patch.Replacement, ) CellarError!Keg { // Build paths var store_buf: [512]u8 = undefined; @@ -199,7 +203,7 @@ pub fn materializeWithCellar( return CellarError.CloneFailed; }; - try relocateKegTree(io, allocator, cellar_path, cellar_type); + try relocateKegTree(io, allocator, cellar_path, cellar_type, extra_replacement); // Write INSTALL_RECEIPT.json for brew compatibility writeInstallReceipt(io, cellar_path, name, version, store_sha256); @@ -376,11 +380,14 @@ pub fn cellarLinksPath(io: std.Io, allocator: std.mem.Allocator, cellar_path: [] /// the local-Cellar copy fallback (`materializeFromLocalCellar`) so /// every keg lands on disk with byte-identical relocation regardless /// of whether it came from the store or a sibling brew install. +/// `extra_replacement` is one caller-resolved substitution applied after the +/// prefix-derived ones, for tokens this module cannot derive on its own. fn relocateKegTree( io: std.Io, allocator: std.mem.Allocator, cellar_path: []const u8, cellar_type: []const u8, + extra_replacement: ?patch.Replacement, ) CellarError!void { const new_prefix = atomic.maltPrefixOrAbort(); @@ -428,13 +435,19 @@ fn relocateKegTree( else => return CellarError.PatchFailed, }; - const text_replacements = [_]patch.Replacement{ - .{ .old = "@@HOMEBREW_PREFIX@@", .new = new_prefix }, - .{ .old = "@@HOMEBREW_CELLAR@@", .new = new_cellar }, - .{ .old = "/opt/homebrew", .new = new_prefix }, - .{ .old = "/usr/local", .new = new_prefix }, - }; - _ = patch.patchTextFiles(io, allocator, cellar_path, &text_replacements) catch |e| { + // Last on purpose: `patchTextFiles` runs sequential passes, so appending + // keeps the substituted path out of the prefix rewrites above. + var text_reps_buf: [5]patch.Replacement = undefined; + text_reps_buf[0] = .{ .old = "@@HOMEBREW_PREFIX@@", .new = new_prefix }; + text_reps_buf[1] = .{ .old = "@@HOMEBREW_CELLAR@@", .new = new_cellar }; + text_reps_buf[2] = .{ .old = "/opt/homebrew", .new = new_prefix }; + text_reps_buf[3] = .{ .old = "/usr/local", .new = new_prefix }; + var text_reps_len: usize = 4; + if (extra_replacement) |r| { + text_reps_buf[4] = r; + text_reps_len = 5; + } + _ = patch.patchTextFiles(io, allocator, cellar_path, text_reps_buf[0..text_reps_len]) catch |e| { std.log.warn("text patching failed for {s}: {s}", .{ cellar_path, @errorName(e) }); }; @@ -597,7 +610,10 @@ pub fn materializeFromLocalCellar( return CellarError.CloneFailed; }; - try relocateKegTree(io, allocator, cellar_path, cellar_type); + // No extra replacement: this keg is copied from a sibling brew install, + // which already resolved every dependency-scoped token at its own + // install time. + try relocateKegTree(io, allocator, cellar_path, cellar_type, null); // Tap-aware receipt: the source-of-truth tap is the sibling // brew install's, not "homebrew/core". `mt list` and friends use diff --git a/src/core/formula.zig b/src/core/formula.zig index f5a66da3..d61b567a 100644 --- a/src/core/formula.zig +++ b/src/core/formula.zig @@ -60,6 +60,79 @@ pub fn pkgVersion(buf: []u8, version: []const u8, revision: i64) ![]const u8 { return std.fmt.bufPrint(buf, "{s}_{d}", .{ version, revision }); } +/// A bottle placeholder whose value is a path inside one of the formula's +/// *dependencies*, resolved against the live prefix. +pub const Placeholder = struct { + token: []const u8, + value: []const u8, +}; + +/// Tokens whose value lives in a dependency's keg. A table because the token +/// set is upstream's: adding one is a data change. +const dependency_placeholders = [_]struct { + token: []const u8, + dep: []const u8, + subpath: []const u8, +}{ + .{ + .token = "@@HOMEBREW_JAVA@@", + .dep = "openjdk", + .subpath = "libexec/openjdk.jdk/Contents/Home", + }, +}; + +/// Runtime dependency names from a keg's own `.brew/.rb`, filled into +/// `out`. Callers holding a parsed `Formula` should use its `dependencies`; +/// this is for paths without one, where the DB rows describe a different +/// version than the keg on disk. +pub fn declaredDependencies(out: [][]const u8, rb_source: []const u8) []const []const u8 { + const decl = "depends_on "; + var n: usize = 0; + var lines = std.mem.tokenizeScalar(u8, rb_source, '\n'); + while (lines.next()) |line| { + if (n == out.len) break; + const trimmed = std.mem.trim(u8, line, " \t\r"); + if (!std.mem.startsWith(u8, trimmed, decl)) continue; + + // Only the quoted form names a formula; `depends_on :xcode` and + // friends are platform predicates, not kegs. + const rest = trimmed[decl.len..]; + if (rest.len == 0 or rest[0] != '"') continue; + const close = std.mem.indexOfScalarPos(u8, rest, 1, '"') orelse continue; + if (std.mem.indexOf(u8, rest[close..], ":build") != null) continue; + + out[n] = rest[1..close]; + n += 1; + } + return out[0..n]; +} + +/// Resolve this formula's dependency-scoped placeholder into `buf`. +/// Null when it declares no matching dependency: guessing would bake a path +/// to a keg the formula never asked for. +pub fn dependencyPlaceholder( + buf: []u8, + prefix: []const u8, + dependencies: []const []const u8, +) ?Placeholder { + for (dependency_placeholders) |entry| { + for (dependencies) |dep| { + const pinned = dep.len > entry.dep.len and + std.mem.startsWith(u8, dep, entry.dep) and + dep[entry.dep.len] == '@'; + if (!std.mem.eql(u8, dep, entry.dep) and !pinned) continue; + + const value = std.fmt.bufPrint( + buf, + "{s}/opt/{s}/{s}", + .{ prefix, dep, entry.subpath }, + ) catch return null; + return .{ .token = entry.token, .value = value }; + } + } + return null; +} + /// Parsed Homebrew formula. Every `[]const u8` and `[]const []const u8` /// field is owned by `_parsed` (either borrowed from the JSON source /// buffer or allocated through the parse arena); valid only until @@ -795,3 +868,122 @@ test "parseFormula releases every auxiliary allocation through deinit" { try testing.expect(formula.service != null); try testing.expect(formula.bottle_files != null); } + +test "dependencyPlaceholder resolves a token against its declared dependency" { + // Wrappers that carry the token are unrunnable until it is substituted, + // and the value is a path into the dependency's keg, not the prefix. + var buf: [256]u8 = undefined; + const deps = [_][]const u8{ "openjdk", "ca-certificates" }; + const got = dependencyPlaceholder(&buf, "/opt/malt", &deps).?; + try testing.expectEqualStrings("@@HOMEBREW_JAVA@@", got.token); + try testing.expectEqualStrings( + "/opt/malt/opt/openjdk/libexec/openjdk.jdk/Contents/Home", + got.value, + ); +} + +test "dependencyPlaceholder keeps the version suffix of a pinned dependency" { + // A pinned variant installs under its own opt/ name; folding it onto the + // unpinned one would point the wrapper at a keg the formula didn't ask for. + var buf: [256]u8 = undefined; + const deps = [_][]const u8{"openjdk@21"}; + const got = dependencyPlaceholder(&buf, "/opt/malt", &deps).?; + try testing.expectEqualStrings( + "/opt/malt/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home", + got.value, + ); +} + +test "dependencyPlaceholder declines a formula with no matching dependency" { + // Most bottles carry no such token; substituting a guessed value would + // be worse than leaving them alone. + var buf: [256]u8 = undefined; + const deps = [_][]const u8{ "pcre2", "zstd" }; + try testing.expect(dependencyPlaceholder(&buf, "/opt/malt", &deps) == null); +} + +test "declaredDependencies reads the names a keg's own formula source declares" { + var out: [8][]const u8 = undefined; + const src = + \\class Benerator < Formula + \\ desc "x" + \\ depends_on "openjdk@11" + \\ depends_on "zstd" + \\end + ; + const got = declaredDependencies(&out, src); + try testing.expectEqual(@as(usize, 2), got.len); + try testing.expectEqualStrings("openjdk@11", got[0]); + try testing.expectEqualStrings("zstd", got[1]); +} + +test "declaredDependencies skips build-only dependencies" { + // Parity with the API `dependencies` field the install path uses: a + // build-only jdk is not present at runtime, so resolving a token against + // it would point at a keg that need not exist. + var out: [8][]const u8 = undefined; + const got = declaredDependencies(&out, + \\ depends_on "cmake" => :build + \\ depends_on "openjdk" + ); + try testing.expectEqual(@as(usize, 1), got.len); + try testing.expectEqualStrings("openjdk", got[0]); +} + +test "declaredDependencies ignores commented and symbol forms" { + var out: [8][]const u8 = undefined; + const got = declaredDependencies(&out, + \\ # depends_on "ghost" + \\ depends_on :xcode + \\ depends_on "real" + ); + try testing.expectEqual(@as(usize, 1), got.len); + try testing.expectEqualStrings("real", got[0]); +} + +test "declaredDependencies stops at the caller's capacity" { + var out: [1][]const u8 = undefined; + const got = declaredDependencies(&out, + \\ depends_on "a" + \\ depends_on "b" + ); + try testing.expectEqual(@as(usize, 1), got.len); +} + +test "declaredDependencies ignores an unterminated quote" { + // A truncated or hand-edited .rb must yield nothing rather than a slice + // running past the closing quote that isn't there. + var out: [4][]const u8 = undefined; + try testing.expectEqual(@as(usize, 0), declaredDependencies(&out, " depends_on \"openjdk\n").len); +} + +test "declaredDependencies handles a source with no dependencies" { + var out: [4][]const u8 = undefined; + try testing.expectEqual(@as(usize, 0), declaredDependencies(&out, "class X < Formula\nend\n").len); +} + +test "declaredDependencies tolerates a zero-capacity buffer" { + var out: [0][]const u8 = undefined; + try testing.expectEqual(@as(usize, 0), declaredDependencies(&out, " depends_on \"openjdk\"\n").len); +} + +test "dependencyPlaceholder declines an empty dependency list" { + var buf: [256]u8 = undefined; + try testing.expect(dependencyPlaceholder(&buf, "/opt/malt", &.{}) == null); +} + +test "dependencyPlaceholder declines rather than truncate a too-small buffer" { + // A truncated path would point somewhere real but wrong; refusing leaves + // the token visible instead, which doctor then reports. + var buf: [8]u8 = undefined; + const deps = [_][]const u8{"openjdk"}; + try testing.expect(dependencyPlaceholder(&buf, "/opt/malt", &deps) == null); +} + +test "dependencyPlaceholder does not mistake a lookalike dependency name" { + // Only the exact name or an `@`-qualified variant counts — a longer name + // that merely starts with it is a different formula. + var buf: [256]u8 = undefined; + const deps = [_][]const u8{"openjdk-doc"}; + try testing.expect(dependencyPlaceholder(&buf, "/opt/malt", &deps) == null); +} diff --git a/src/core/relocated_store.zig b/src/core/relocated_store.zig index e3050d88..88b83fa2 100644 --- a/src/core/relocated_store.zig +++ b/src/core/relocated_store.zig @@ -38,7 +38,11 @@ pub const RelocatedStoreError = error{ /// v2: the Mach-O patcher learned to drop LC_RPATHs that relocation collapses /// onto one prefix; v1 entries were snapshotted before that and ship the /// duplicate that makes dyld abort at launch. -pub const RELOC_LOGIC_VERSION: u32 = 2; +/// +/// v3: relocation gained dependency-scoped placeholder substitution. v2 entries +/// cached the literal token, so an affected wrapper restored from one stays +/// broken however many times it is reinstalled. +pub const RELOC_LOGIC_VERSION: u32 = 3; /// Reject anything that is not exactly 64 lowercase hex characters. /// Run this before forming any path so traversal sequences (`..`, `/`) and diff --git a/tests/cellar_test.zig b/tests/cellar_test.zig index c24ea8ed..fd6aab18 100644 --- a/tests/cellar_test.zig +++ b/tests/cellar_test.zig @@ -73,6 +73,28 @@ fn createBottleFixture(allocator: std.mem.Allocator, prefix: []const u8, sha: [] } } +/// A keg with one `bin/` wrapper carrying a dependency-scoped placeholder — +/// a token whose value is a path inside another keg, so relocation cannot +/// derive it from the prefix alone. +fn createPlaceholderBottleFixture(allocator: std.mem.Allocator, prefix: []const u8, sha: []const u8, name: []const u8, ver_dir: []const u8) !void { + const keg = try std.fmt.allocPrint(allocator, "{s}/store/{s}/{s}/{s}", .{ prefix, sha, name, ver_dir }); + defer allocator.free(keg); + try test_io.cwd().createDirPath(std.Options.debug_io, keg); + + const bin_dir = try std.fmt.allocPrint(allocator, "{s}/bin", .{keg}); + defer allocator.free(bin_dir); + try test_io.makeDirAbsolute(std.Options.debug_io, bin_dir); + + const script_path = try std.fmt.allocPrint(allocator, "{s}/bin/wrapper", .{keg}); + defer allocator.free(script_path); + const f = try test_io.createFileAbsolute(std.Options.debug_io, script_path, .{}); + try f.writeStreamingAll( + std.Options.debug_io, + "#!/bin/bash\nDEP_HOME=\"${DEP_HOME:-@@HOMEBREW_JAVA@@}\" exec @@HOMEBREW_CELLAR@@/pkg/1.0/libexec/bin/wrapper \"$@\"\n", + ); + f.close(std.Options.debug_io); +} + fn setupMaltDirs(allocator: std.mem.Allocator, prefix: []const u8) !void { const dirs = [_][]const u8{ "store", "Cellar", "opt", "bin", "lib" }; for (dirs) |d| { @@ -116,6 +138,7 @@ test "materialize handles version with revision suffix" { "pcre2", "10.47", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -177,6 +200,7 @@ test "materialize replaces a pre-existing Cellar/{name}/{version} directory (gh# "lld@21", "21.1.8_1", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -210,6 +234,7 @@ test "materialize handles exact version match (no revision)" { "jq", "1.7.1", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -242,7 +267,8 @@ test "placeholder substitution runs for relocatable bottles" { "rel123", "stow", "2.4.1", - ":any", // relocatable — the bug scenario + ":any", // relocatable — the bug scenario, + null, ); defer testing.allocator.free(keg.path); @@ -259,6 +285,86 @@ test "placeholder substitution runs for relocatable bottles" { try testing.expect(std.mem.indexOf(u8, content, prefix) != null); } +test "relocation substitutes the caller-resolved dependency placeholder" { + // Without this the token survives into the installed wrapper, which then + // resolves to a path that does not exist and fails at launch. + const prefix = try createTestDir(testing.allocator); + defer { + test_io.deleteTreeAbsolute(std.Options.debug_io, prefix) catch {}; + testing.allocator.free(prefix); + } + + try setupMaltDirs(testing.allocator, prefix); + try createPlaceholderBottleFixture(testing.allocator, prefix, "dep123", "pkg", "1.0"); + + const old_env = setMaltPrefix(prefix); + defer restoreMaltPrefix(old_env); + + var value_buf: [512]u8 = undefined; + const value = try std.fmt.bufPrint( + &value_buf, + "{s}/opt/openjdk/libexec/openjdk.jdk/Contents/Home", + .{prefix}, + ); + + const keg = try cellar_mod.materializeWithCellar( + std.Options.debug_io, + testing.allocator, + prefix, + "dep123", + "pkg", + "1.0", + ":any", + .{ .old = "@@HOMEBREW_JAVA@@", .new = value }, + ); + defer testing.allocator.free(keg.path); + + var script_buf: [512]u8 = undefined; + const script_path = try std.fmt.bufPrint(&script_buf, "{s}/bin/wrapper", .{keg.path}); + const content = try readFile(testing.allocator, script_path); + defer testing.allocator.free(content); + + try testing.expect(std.mem.indexOf(u8, content, "@@HOMEBREW_JAVA@@") == null); + try testing.expect(std.mem.indexOf(u8, content, value) != null); +} + +test "relocation leaves an unresolved dependency placeholder in place" { + // Substituting a guessed value would point the wrapper at a keg the + // formula never asked for — worse than leaving the token visible. + const prefix = try createTestDir(testing.allocator); + defer { + test_io.deleteTreeAbsolute(std.Options.debug_io, prefix) catch {}; + testing.allocator.free(prefix); + } + + try setupMaltDirs(testing.allocator, prefix); + try createPlaceholderBottleFixture(testing.allocator, prefix, "dep456", "pkg", "1.0"); + + const old_env = setMaltPrefix(prefix); + defer restoreMaltPrefix(old_env); + + const keg = try cellar_mod.materializeWithCellar( + std.Options.debug_io, + testing.allocator, + prefix, + "dep456", + "pkg", + "1.0", + ":any", + null, + ); + defer testing.allocator.free(keg.path); + + var script_buf: [512]u8 = undefined; + const script_path = try std.fmt.bufPrint(&script_buf, "{s}/bin/wrapper", .{keg.path}); + const content = try readFile(testing.allocator, script_path); + defer testing.allocator.free(content); + + try testing.expect(std.mem.indexOf(u8, content, "@@HOMEBREW_JAVA@@") != null); + // The prefix-derived tokens still relocate — only the extra one is withheld. + try testing.expect(std.mem.indexOf(u8, content, "@@HOMEBREW_CELLAR@@") == null); +} + test "placeholder substitution replaces multiple tokens in single file" { const prefix = try createTestDir(testing.allocator); defer { @@ -280,6 +386,7 @@ test "placeholder substitution replaces multiple tokens in single file" { "pkg", "1.0", "", + null, ); defer testing.allocator.free(keg.path); @@ -332,6 +439,7 @@ test "files with no placeholders are left unchanged" { "noop", "1.0", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -379,6 +487,7 @@ test "binary files are skipped by text patching without error" { "binpkg", "1.0", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -446,6 +555,7 @@ test "materializeWithCellar short-circuits when the relocated cache has the sha" "cached", "1.0", ":any", + null, ); testing.allocator.free(keg_pre.path); try relocated_mod.save(std.Options.debug_io, testing.allocator, prefix, valid_test_sha, "cached", "1.0"); @@ -464,6 +574,7 @@ test "materializeWithCellar short-circuits when the relocated cache has the sha" "cached", "1.0", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -498,6 +609,7 @@ test "materializeWithCellar populates the relocated cache after a cold install" "snap", "0.1", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -574,6 +686,7 @@ test "materializeWithCellar rebuilds a cached keg whose binary would abort dyld" "poisoned", "1.0", ":any", + null, ); testing.allocator.free(first.path); try testing.expect(relocated_mod.has(std.Options.debug_io, prefix, valid_test_sha)); @@ -606,6 +719,7 @@ test "materializeWithCellar rebuilds a cached keg whose binary would abort dyld" "poisoned", "1.0", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -636,6 +750,7 @@ test "materializeWithCellar trusts a snapshot it already verified" { "trusted", "1.0", ":any", + null, ); testing.allocator.free(first.path); try testing.expect(relocated_mod.isVerified(std.Options.debug_io, prefix, valid_test_sha)); @@ -661,6 +776,7 @@ test "materializeWithCellar trusts a snapshot it already verified" { "trusted", "1.0", ":any", + null, ); defer testing.allocator.free(keg.path); @@ -706,6 +822,7 @@ test "materializeWithCellar refuses a keg whose binary kept an unsubstituted pla "unpatched", "2.0", ":any", + null, )); // A keg that cannot load is not left installed, and never gets cached. @@ -763,6 +880,7 @@ test "failed materialize cleans up empty Cellar/{name}/ parent dir" { "ghost", "0.0.1", ":any", + null, ); try testing.expectError(cellar_mod.CellarError.CloneFailed, result); @@ -802,6 +920,7 @@ test "failed materialize leaves sibling versions untouched" { "keeper", "2.0", ":any", + null, ); try testing.expectError(cellar_mod.CellarError.CloneFailed, result); @@ -964,6 +1083,7 @@ test "materialize rewrites @@HOMEBREW_PREFIX@@ in Mach-O rpath for :any bottle" name, version, ":any", + null, ); defer testing.allocator.free(keg.path); @@ -1147,6 +1267,7 @@ test "P9: materialize patches @@HOMEBREW_PREFIX@@ in EVERY fat-binary arch slice name, version, ":any", + null, ); defer testing.allocator.free(keg.path); @@ -1238,6 +1359,7 @@ test "materialize rewrites @@HOMEBREW_CELLAR@@ in Mach-O rpath for :any bottle" name, version, ":any_skip_relocation", + null, ); defer testing.allocator.free(keg.path); @@ -1430,6 +1552,7 @@ test "warm cache-hit reinstall re-pours a wiped overlay config" { "fc", "1.0", ":any", + null, ); testing.allocator.free(keg_cold.path); try testing.expect(relocated_mod.has(std.Options.debug_io, prefix, valid_test_sha)); @@ -1457,6 +1580,7 @@ test "warm cache-hit reinstall re-pours a wiped overlay config" { "fc", "1.0", ":any", + null, ); testing.allocator.free(keg_warm.path); diff --git a/tests/install_warm_path_test.zig b/tests/install_warm_path_test.zig index ae1bba90..6f90bcd2 100644 --- a/tests/install_warm_path_test.zig +++ b/tests/install_warm_path_test.zig @@ -111,6 +111,7 @@ test "install → uninstall → reinstall takes the relocated cache short-circui "warmpkg", "1.0", ":any", + null, ); defer testing.allocator.free(keg.path); } @@ -138,6 +139,7 @@ test "install → uninstall → reinstall takes the relocated cache short-circui "warmpkg", "1.0", ":any", + null, ); defer testing.allocator.free(keg.path); try testing.expect(pathExists(keg.path)); @@ -173,6 +175,7 @@ test "non-APFS-style cache miss still allows a successful pipeline reinstall" { "nocache", "0.1", ":any", + null, ); defer testing.allocator.free(keg.path); } @@ -195,6 +198,7 @@ test "non-APFS-style cache miss still allows a successful pipeline reinstall" { "nocache", "0.1", ":any", + null, ); defer testing.allocator.free(keg.path); // Pipeline rebuilt the keg AND restored the cache — warm reinstalls From 0049a3073abfd55ab217f37c7cfc1bcc7c4b3d3e Mon Sep 17 00:00:00 2001 From: indaco Date: Sun, 26 Jul 2026 22:53:13 +0200 Subject: [PATCH 2/2] fix(doctor): report unsubstituted placeholders in text files The placeholder check only inspected Mach-O load commands for two tokens, so a wrapper script that kept a relocation token read as healthy. That is how a keg could install successfully, fail at launch, and still pass a doctor run. The scan now also covers text files and matches any Homebrew token rather than the two malt substitutes: the patcher's verify gate must not fail an install over a token it does not know, but doctor only warns, and an unresolved token is exactly the finding worth surfacing. The check keeps its existing id so scripted consumers are unaffected. --- ...ndency-scoped-placeholder-unsubstituted.sh | 36 ++++++---- .../regressions/install_force_sweeps_tap.sh | 2 +- src/cli/doctor.zig | 67 ++++++++++++++++--- 3 files changed, 82 insertions(+), 23 deletions(-) diff --git a/scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh b/scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh index 9782bef5..f1935f86 100755 --- a/scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh +++ b/scripts/regressions/dependency-scoped-placeholder-unsubstituted.sh @@ -6,12 +6,15 @@ # path that does not exist and failed at launch, behind an install that # reported success and a `doctor` run that reported the prefix healthy. # -# Two halves have to stay fixed, and each fails differently: -# 1. relocation must accept and apply a caller-resolved replacement, and the -# install path must resolve it from the formula's declared dependencies; -# 2. RELOC_LOGIC_VERSION must have moved past 2, or `store-relocated/v2/` +# Four things have to stay fixed, and each fails differently: +# 1. relocation must accept and apply a caller-resolved replacement; +# 2. every command that materialises a keg must resolve it — install, migrate +# and rollback each reach relocation by a different route, and two of them +# were missed on the first pass; +# 3. RELOC_LOGIC_VERSION must have moved past 2, or `store-relocated/v2/` # still serves kegs snapshotted with the literal token and the fix never -# reaches anyone who already installed an affected bottle. +# reaches anyone who already installed an affected bottle; +# 4. doctor must scan text files, or the whole class stays invisible. # # Behavioural guards live in colocated `test {}` blocks; this script asserts # the wiring is present statically, then runs the two test binaries and judges @@ -41,18 +44,27 @@ if ! grep -q 'extra_replacement' "$ROOT/src/core/cellar.zig"; then exit 1 fi -# 3. Every path that materialises a bottle from the API must resolve the token; -# the formula is in scope at each. `migrate` shares the bottle path with -# `install`, and reaching for the short `materialize()` wrapper there is the -# easy way to silently reintroduce the bug for migrated kegs. -for site in src/cli/install/download.zig src/cli/migrate/keg.zig; do +# 3. EVERY path that materialises a keg must resolve the token. Reaching for +# the short `materialize()` wrapper at any of these is the easy way to +# silently reintroduce the bug for one command while the others stay fixed — +# which is exactly how migrate and rollback were missed the first time. +# (`upgrade` is covered transitively: it routes through download.zig.) +for site in src/cli/install/download.zig src/cli/migrate/keg.zig src/cli/rollback.zig; do if ! grep -q 'dependencyPlaceholder' "$ROOT/$site"; then echo "FAIL: $site no longer resolves the placeholder from the formula" >&2 exit 1 fi done -# 4. Cached kegs relocated before the fix hold the literal token. Serving one +# 4. The safety net: doctor must scan text files, not just Mach-O. Without it a +# prefix with an unresolved token still reports healthy, which is what let +# the original defect ship unnoticed. +if ! grep -q 'textCarriesPlaceholder' "$ROOT/src/cli/doctor.zig"; then + echo "FAIL: doctor no longer scans text files for unsubstituted tokens" >&2 + exit 1 +fi + +# 5. Cached kegs relocated before the fix hold the literal token. Serving one # back re-breaks a fixed binary, so the cache version must be past 2. version=$(grep -oE 'RELOC_LOGIC_VERSION: u32 = [0-9]+' "$ROOT/src/core/relocated_store.zig" | grep -oE '[0-9]+$' || true) @@ -65,7 +77,7 @@ if ((version < 3)); then exit 1 fi -# 5. Behavioural guards. If either test block is deleted the runs below would +# 6. Behavioural guards. If either test block is deleted the runs below would # silently pass, so assert they are present first. RESOLVE_TEST="dependencyPlaceholder resolves a token against its declared dependency" PINNED_TEST="dependencyPlaceholder keeps the version suffix of a pinned dependency" diff --git a/scripts/regressions/install_force_sweeps_tap.sh b/scripts/regressions/install_force_sweeps_tap.sh index 21fb0ba3..def9a7c8 100755 --- a/scripts/regressions/install_force_sweeps_tap.sh +++ b/scripts/regressions/install_force_sweeps_tap.sh @@ -114,7 +114,7 @@ if echo "$DOCTOR_OUT" | grep -qE "keg\(s\) in DB but missing on disk"; then echo "$DOCTOR_OUT" >&2 fail "doctor reports Missing kegs after tap-path force-sweep" fi -if echo "$DOCTOR_OUT" | grep -qE "package\(s\) ship Mach-O"; then +if echo "$DOCTOR_OUT" | grep -qE "package\(s\) ship file\(s\) with unpatched @@HOMEBREW"; then echo "$DOCTOR_OUT" >&2 fail "doctor reports Mach-O placeholders after tap-path force-sweep" fi diff --git a/src/cli/doctor.zig b/src/cli/doctor.zig index 97ec30a5..15101209 100644 --- a/src/cli/doctor.zig +++ b/src/cli/doctor.zig @@ -1110,9 +1110,9 @@ fn checkMachOPlaceholders(ctx: CheckCtx, name: []const u8) CheckResult { // would just duplicate the first row of that list. break :blk std.fmt.bufPrint( &msg_buf, - "{d} package(s) ship Mach-O file(s) with unpatched @@HOMEBREW_* placeholders. Reinstall the affected packages.", + "{d} package(s) ship file(s) with unpatched @@HOMEBREW_* placeholders. Reinstall the affected packages.", .{groups.count()}, - ) catch "Mach-O files with unpatched @@HOMEBREW_* placeholders found."; + ) catch "Files with unpatched @@HOMEBREW_* placeholders found."; } const first_key = first_key: { var it = groups.iterator(); @@ -1120,9 +1120,9 @@ fn checkMachOPlaceholders(ctx: CheckCtx, name: []const u8) CheckResult { }; break :blk std.fmt.bufPrint( &msg_buf, - "{d} package(s) ship Mach-O file(s) with unpatched @@HOMEBREW_* placeholders (first: {s}). Reinstall the affected packages.", + "{d} package(s) ship file(s) with unpatched @@HOMEBREW_* placeholders (first: {s}). Reinstall the affected packages.", .{ groups.count(), first_key }, - ) catch "Mach-O files with unpatched @@HOMEBREW_* placeholders found."; + ) catch "Files with unpatched @@HOMEBREW_* placeholders found."; }; printCheck(name, .err_status, msg); armVerboseHint(); @@ -1310,10 +1310,19 @@ fn checkLocalSources(ctx: CheckCtx, name: []const u8) CheckResult { return .warn_status; } -/// True if `rel_path` inside `base_dir` is a Mach-O binary with at least one -/// load command that still contains `@@HOMEBREW_PREFIX@@` or -/// `@@HOMEBREW_CELLAR@@`. Any I/O or parser error is treated as "not bad" — -/// doctor's placeholder check is best-effort. +/// True when `content` is patchable text still carrying a relocation token. +/// Matches `@@HOMEBREW_` generically: doctor only warns, so an unknown token +/// is worth surfacing rather than ignoring. The NUL sniff mirrors +/// `patchTextFiles` so doctor never flags a file the patcher would skip. +fn textCarriesPlaceholder(content: []const u8) bool { + const check_len = @min(content.len, 8192); + if (std.mem.findScalar(u8, content[0..check_len], 0) != null) return false; + return std.mem.indexOf(u8, content, "@@HOMEBREW_") != null; +} + +/// True if `rel_path` still carries a relocation token, in a Mach-O load +/// command or in text. I/O and parser errors count as "not bad": doctor's +/// placeholder check is best-effort. fn hasUnpatchedPlaceholder( io: std.Io, allocator: std.mem.Allocator, @@ -1326,10 +1335,22 @@ fn hasUnpatchedPlaceholder( var magic: [4]u8 = undefined; const n = file.readPositionalAll(io, &magic, 0) catch return false; if (n < 4) return false; - if (!parser.isMachO(&magic)) return false; + const is_macho = parser.isMachO(&magic); - // Re-read the full file — parser needs the whole buffer. const stat = file.stat(io) catch return false; + + // A wrapper script that kept a token is as broken as a dylib that did. + // The 10MB cap matches `patchTextFiles`: larger files were never patched. + if (!is_macho) { + if (stat.size == 0 or stat.size > 10 * 1024 * 1024) return false; + const text = allocator.alloc(u8, stat.size) catch return false; + defer allocator.free(text); + const got = file.readPositionalAll(io, text, 0) catch return false; + if (got < text.len) return false; + return textCarriesPlaceholder(text); + } + + // Re-read the full file — parser needs the whole buffer. if (stat.size > 512 * 1024 * 1024) return false; // skip pathologically large files const data = allocator.alloc(u8, stat.size) catch return false; defer allocator.free(data); @@ -1832,3 +1853,29 @@ test "runChecks: an info_status finding counts as neither warning nor error" { try testing.expectEqual(@as(u32, 0), tally.warnings); try testing.expectEqual(@as(u32, 0), tally.errors); } + +test "textCarriesPlaceholder flags a wrapper script that kept a token" { + // The failure this check exists to catch is a launcher that still points + // at a placeholder, which no Mach-O scan can see. + // No shebang in the fixture: the spawn-invariant guard rejects shell + // literals anywhere under src/, and the token is what's under test. + try testing.expect(textCarriesPlaceholder( + "DEP=\"${DEP:-@@HOMEBREW_JAVA@@}\" exec real \"$@\"\n", + )); +} + +test "textCarriesPlaceholder flags a token this malt does not resolve" { + // Deliberately not restricted to the tokens malt substitutes: an + // unknown one is exactly the case worth surfacing, and doctor only warns. + try testing.expect(textCarriesPlaceholder("prefix=@@HOMEBREW_SOMETHING_NEW@@\n")); +} + +test "textCarriesPlaceholder ignores a relocated script" { + try testing.expect(!textCarriesPlaceholder("exec /opt/malt/bin/real\n")); +} + +test "textCarriesPlaceholder ignores binary content" { + // patchTextFiles skips anything with a NUL in the first 8KB, so flagging + // one would report a defect malt has no way to repair. + try testing.expect(!textCarriesPlaceholder("\x7fELF\x00\x00@@HOMEBREW_PREFIX@@")); +}