amadan
The replicator's sixteenth build: the fifteenth review's findings
A link's relative target is stat'ed as concatenated to the link's directory, never cleaned, so a ".." after a link is resolved as the kernel resolves it and a link to a directory outside the tree is a problem, not silence (Opus P3-1, a regression of the fifteenth build); the impostor case accepts a flat pair it can spell; the owned line of the rendering is quoted whole. Dispositions "Review fifteen" (Astra's report pending) and Opus's report.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
6 files changed,
+181
−10
docs/superpowers/specs/DISPOSITIONS-metered-replicator.md+15 −2docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review15.md+110 −0internal/ops/litestream_metered.go+1 −1internal/ops/litestream_metered_test.go+24 −4internal/ops/litestream_owners.go+8 −3internal/ops/litestream_owners_test.go+23 −0
CI passed — run details
diff --git a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md| index 03ce0774e..ddd2ad742 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-metered-replicator.md |
| @@ -265,7 +265,7 @@ Reports: `OPUS-REVIEW-metered-replicator-review12.md`, `CODEX-FINDINGS-metered-r |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| -| Opus P3-1, Astra 5 (P2): the link entry was not a safe acceptance key or encoding. | Fixed in the thirteenth build as `path -> "target"` with the target quoted, `Accept` comparing a link entry as written and a plain path as cleaned [the fourteenth build made it `link "path" -> "target"`, both halves quoted; the fifteenth build's Accept fixture (`sub/live.sqlite` against `sub/./live.sqlite`) is the one that tells as-written from cleaned, the fourteenth's `x/..` case did not]. [Review thirteen: a regular file whose own name spelt the pair read as the accepted link entry; the fourteenth build quotes both halves behind the word `link`, which no absolute path begins with, so the encoding is one to one.] Relative targets are rendered as `Readlink` gives them (the classification follows the kernel's resolution, the entry names the spelling). | `litestream_owners.go` `linkEntry`, `isLinkEntry`; `litestream_metered.go` `MeteredFleetStore` | `TestAcceptComparesALinkEntryAsWritten` (the pair accepted builds the store; the link re-pointed to the same file through `x/..` refuses it until the new pair is accepted); `TestStrictTreeNamesALinkToAFile` (a target spelt through `..` is named as written); each fails with the target cleaned, or with link entries cleaned by `Accept` (mutation runs) | |
| +| Opus P3-1, Astra 5 (P2): the link entry was not a safe acceptance key or encoding. | Fixed in the thirteenth build as `path -> "target"` with the target quoted, `Accept` comparing a link entry as written and a plain path as cleaned [the fourteenth build made it `link "path" -> "target"`, both halves quoted; the fifteenth build's Accept fixture (`sub/live.sqlite` against `sub/./live.sqlite`) is the one that tells as-written from cleaned, the fourteenth's `x/..` case did not]. [Review thirteen: a regular file whose own name spelt the pair read as the accepted link entry; the fourteenth build quotes both halves behind the word `link`, which no absolute path begins with, so the encoding is one to one.] Relative targets are rendered as `Readlink` gives them (the entry names the spelling; from the sixteenth build the classification stats that spelling concatenated to the link's directory, uncleaned, which is the kernel's resolution of it). | `litestream_owners.go` `linkEntry`, `isLinkEntry`; `litestream_metered.go` `MeteredFleetStore` | `TestAcceptComparesALinkEntryAsWritten` (the pair accepted builds the store; the link re-pointed to the same file through `x/..` refuses it until the new pair is accepted); `TestStrictTreeNamesALinkToAFile` (a target spelt through `..` is named as written); each fails with the target cleaned, or with link entries cleaned by `Accept` (mutation runs at the time; from the fifteenth build the as-written case is `sub/./live.sqlite`) | |
| | Opus P3-2: the checks on one file were not required to see one inode. | Fixed: the facts kept are the first check's, and a later check whose `Dev`/`Ino` differ refuses the file ("changed under the look"). No deterministic test: the case is a rename between two checks of one inventory; stated. | `litestream_owners.go` `fleetInventory` | | |
| | Opus P3-3, Astra 11: the every-claim test could not tell "every check" from "the tree check only", and its comment called the needed layout impossible; the root/platform test is vacuous as root; the sealed-link case is skipped as root. | Fixed: the layout exists (the tree's user's file in another user's directory inside the tree: the tree check accepts, the file check refuses the directory) and is the test's second case; the root/platform test skips as root with a message. The sealed case stays skipped as root (root can look at anything), stated. | tests | `TestEveryClaimOnAFileMustPass`; fails with tree checks alone whenever a tree claim exists (mutation run) | |
| | Opus P3-4: a `Readlink` error was dropped. | Fixed: ENOENT and EINVAL (no longer a link) are churn; any other error is a problem. | `litestream_owners.go` `strictTreeDBs` | no deterministic test (a link that cannot be read by this process while its directory can be listed is not arranged); stated | |
| @@ -296,10 +296,23 @@ Reports: `OPUS-REVIEW-metered-replicator-review14.md`, `CODEX-FINDINGS-metered-r |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| | Opus P3-2, Astra 5 (P2): `render()` wrote names raw, so a name with a line break read as two entries and two as one, and the running fleet's drift comparison could be defeated. | Fixed: every entry in `render()` is quoted (`strconv.Quote`), owned paths included. | `litestream_metered.go` `render` | `TestRenderQuotesEveryEntry`; fails with entries written raw (mutation run) | |
| -| Opus P3-1, Astra 7 (P2): the second `Readlink` missed a link re-pointed and put back, and dropped a second-read error. | Fixed: the second read is gone; the target is classified by the spelling the entry names (a `Stat` of the target as read, from the link's directory when relative), so the entry and what was looked at are one string whatever the link points to meanwhile. What that spelling resolves to is not pinned, as stated before. | `litestream_owners.go` `strictTreeDBs` | no deterministic test (a tenant-timed re-point around one `Stat`); stated | |
| +| Opus P3-1, Astra 7 (P2): the second `Readlink` missed a link re-pointed and put back, and dropped a second-read error. | Fixed: the second read is gone; the target is classified by the spelling the entry names (a `Stat` of the target as read, from the link's directory when relative). [Review fifteen: the fifteenth build joined and so cleaned that spelling, which is not the kernel's resolution of a `..` after a link; the sixteenth build concatenates, uncleaned.] What that spelling resolves to is not pinned, as stated before. | `litestream_owners.go` `strictTreeDBs` | no deterministic test (a tenant-timed re-point around one `Stat`); stated | |
| | Opus P3-3, Astra 12: the Accept test no longer told as-written from cleaned. | Fixed: the fixture accepts `sub/live.sqlite` and re-points to `sub/./live.sqlite`, which cleaning folds to the accepted entry. | tests | `TestAcceptComparesALinkEntryAsWritten`; fails with link entries cleaned by `Accept` (mutation run) | |
| | Opus claims: the "passes over on purpose" list; the newline-fixture sentence ("a temp root cannot hold it": false). | Corrected: the list names a link that is something else when read; the fixture makes a target with a line break and asserts it quoted. | `litestream_owners.go`, `litestream_owners_test.go` | `TestStrictTreeNamesALinkToAFile` | |
| | Astra 1 to 4, 6, 8 to 11: the deep items, the hibernating aliases and skipped-file claims, required coverage that can vanish, the box-wide refusal, the uid binding, the remaining test gaps (shutdown, watch, attribution, interleavings, root). | Open, as before, stated where each applies; the box-wide refusal and the uid binding are decisions before wiring. | | | |
| | Claims (both): this file's rows for the thirteenth-build link entry, the "display" sentence, the Readlink race "fixed", the EINVAL exemption; the unconditional witness sentences at `Replaces`, `inheritHold` and `Open` (a fleet made stale by the final look skips the witness). | Corrected in place; the three witness sentences now say "unless already stale, or made stale by the final look". | this file, `litestream_metered.go` | | |
| Package tests pass under `/tmp` and `~/.cache` (the author's runs, not as root). The full gate on this tip is next; the fifteenth build has not been read by a reviewer. |
| + |
| +## Review fifteen (reviewed `03b19014`; Opus accepted with fixes, Astra's report pending at the sixteenth build's commit) |
| + |
| +Report: `OPUS-REVIEW-metered-replicator-review15.md` (Astra's, when it lands, is copied in by the next commit). Opus found review fourteen's three findings closed and one regression in the fifteenth build's own fix: the relative target was joined with `filepath.Join`, which cleans a `..` lexically, where the kernel resolves it physically through whatever the component before it led to; a link whose target steps up through an outside link to a directory was therefore stat'ed beside the link, found nothing, and passed over. The sixteenth build: |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P3-1: a relative target resolved lexically, not as the kernel resolves it; a link to a directory outside the tree passed over in silence; a tree root reached through a link not equivalent in general. | Fixed: the spelling is concatenated to the link's directory, never cleaned, and stat'ed as such, so the kernel resolves the `..` physically; the link itself is still never re-read. | `litestream_owners.go` `strictTreeDBs` | `TestStrictTreeNamesALinkToAFile` (`current -> "../o/lnk/../dbs"` with `lnk` an outside link to a directory: a problem); fails with the join (mutation run) | |
| +| Opus P3-2: the impostor case no longer told the unprefixed encoding (the accepted pair's target had a separator). | Fixed: the impostor step accepts a flat pair (`current2 -> "live2.sqlite"`) and names the impostor exactly so. | tests | `TestAcceptComparesALinkEntryAsWritten`; fails with the unprefixed encoding (mutation run) | |
| +| Opus P3-3: the owned line's path quoting was untested and the owner triple raw. | Fixed: the whole owned line is quoted and the test has an owned path with a line break. | `litestream_metered.go` `render` | `TestRenderQuotesEveryEntry`; fails with the owned line raw (mutation run) | |
| +| Claims: "classified by where it leads (one Stat)"; the test message "through a .."; this file's "follows the kernel's resolution" and "one string"; the review-thirteen row's mutation claim. | Corrected in place. | | | |
| + |
| +Astra 1 to 4, 6, 8 to 11 stay open as before. Package tests pass under `/tmp` and `~/.cache` (the author's runs, not as root). The full gate on this tip is next; the sixteenth build has not been read by a reviewer. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review15.md b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review15.md| new file mode 100644 |
| index 000000000..b76bf3856 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-metered-replicator-review15.md |
| @@ -0,0 +1,110 @@ |
| +# Fifteenth review: feat/carloku-metered-replicator-20261001 at `03b19014` |
| + |
| +**Verdict: accept with fixes.** All three review-fourteen Opus findings are closed, and so are Astra 5 and 7. One fix brings a new defect: a link is now classified by joining its relative target to the walker's path (`filepath.Join`), which cleans away `..`. The kernel does not resolve the link that way. Through a link outside the walked tree, or a tree root reached through a link, a link to a directory can now be passed over in silence. Before this build, `os.Stat(p)` caught it. Separately, the fixture change leaves the impostor case unable to detect the encoding regression it was written for. |
| + |
| +The fifteenth build changes only: |
| +- `render()` (`litestream_metered.go:673-690`); |
| +- the link classification in `strictTreeDBs` (`litestream_owners.go:685-694`); |
| +- the "passes over on purpose" comment, three witness sentences (`Replaces`, `inheritHold`, `Open`), two tests and the dispositions. |
| + |
| +The registry, admission, Close, the latch, lineage, the claim checks and the chain climb are untouched. A–Z therefore stand as reviews thirteen and fourteen answered them. I read source only through `git show 03b19014:` and ran nothing. |
| + |
| +## Review fourteen, finding by finding |
| + |
| +| Finding | At `03b19014` | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1 / Astra 7:** the second `Readlink` missed a re-point-and-restore, and dropped second-read errors | **Closed.** The second read is gone. `Stat` runs on the spelling the entry names (`litestream_owners.go:690-694`), so the A→F→A swap around `Stat(p)` no longer applies. But the relative join is lexical (P3-1 below). | **Yes.** No test, as stated. | |
| +| **Opus P3-2 / Astra 5:** `render()` was not injective | **Closed** (`litestream_metered.go:679`, `:686-688`). | **No** for the list sections (`TestRenderQuotesEveryEntry`). **Yes** for quoting the owned path: nothing checks it. | |
| +| **Opus P3-3 / Astra 12 (first part):** the `Accept` test could not tell as-written from cleaned | **Closed.** Under whole-entry cleaning, `sub/./live.sqlite` folds to the accepted entry, so the build would succeed and the test fails. | **No.** But the same test's impostor case is now vacuous (P3-2 below). | |
| +| **Opus claims** | The "passes over on purpose" list now names the EINVAL case. The newline-fixture sentence is corrected, and a fixture is added (`litestream_owners_test.go:706`). | n/a | |
| +| **Astra 1–4, 6, 8–11** | **Open.** The code is unchanged. | n/a | |
| +| **Astra 12 (other mutations)** | **Open:** shutdown, watch, recorder swap, the pre-witness stale check, the latch recheck, `prepareShadowState`, the two inode checks, the 300 ms Close check and the wall-clock clean-start test. | Yes, as listed in review fourteen. | |
| +| **Astra claims** | `:271` (EINVAL), `:283` ("display"), `:285` ("fixed") and the three witness sentences are corrected. `:268` is only partly corrected (see Claims). | n/a | |
| + |
| +## Findings |
| + |
| +### P3-1. A relative link target is resolved lexically, not as the kernel resolves it (BB2): a regression |
| + |
| +**Where:** `litestream_owners.go:691-694`. |
| + |
| +`filepath.Join(dir, dest)` cleans `x/..` before the `Stat`. The kernel resolves `x` first, then goes to the parent of where `x` led. `walkWithoutLinks` (`:746-757`) states exactly this distinction. |
| + |
| +**Scenario 1: a link outside the tree.** |
| +1. Tenant T has its own user and a tree at `/srv/T/state`. T also writes `/srv/T/other` and `/srv/T/hidden`. |
| +2. T makes `/srv/T/other/lnk -> /srv/T/hidden/sub`. |
| +3. T makes `state/current -> "../other/lnk/../dbs"`. |
| +4. The walker stats `/srv/T/other/dbs`. That is ENOENT, so the link "leads nowhere" and is passed over in silence. |
| +5. T's software follows the link to `/srv/T/hidden/dbs`, a directory full of SQLite files. |
| +6. At `ec55b639`, `Stat(p)` went through the kernel, so this was a problem: "a link that may lead to a database". Now it is in no list. |
| + |
| +**Variant (an accepted pair can mean something else):** T creates a plain file at `/srv/T/other/dbs`. |
| +- The walker sees a regular file. The entry `link "…/current" -> "../other/lnk/../dbs"` is skipped, and the operator accepts it. |
| +- The kernel still reaches the directory, and the accepted pair stays accepted across rebuilds. |
| + |
| +**Scenario 2: a tree root reached through a link.** Unit roots can be reached through a link (`:410`, systemd's `/var/lib/X -> private/X`). |
| +- At depth 0, `dir/..` is lexical, while the kernel's `..` is physical. |
| +- Under systemd's mirrored layout, most cases fail closed (the lexical path hits a real directory and is reported as a problem) or reach the same object. |
| +- Equivalence is not established in general. |
| + |
| +Inside the tree the gap closes, because an intermediate link to a directory is itself reported as a problem. |
| + |
| +**Bound:** |
| +- It needs a link the walker never classifies. |
| +- litestream never opens these links; it opens only `dbs`, which are regular files. |
| +- So this is a hole in "nothing passed over in silence", not in what gets replicated or billed. |
| + |
| +**Smallest fix:** concatenate without cleaning: `spelled = dir + string(filepath.Separator) + dest`. The kernel then resolves `..` physically, including through a linked root, and the link itself is still not re-read. An alternative is `unix.Fstatat` on an fd for `dir`. |
| + |
| +Add a fixture: an outside link `lnk` to a directory, and `current -> "../o/lnk/../d"`, expected to be reported as a problem. That test fails against `filepath.Join`. |
| + |
| +### P3-2. The impostor case no longer detects the unprefixed encoding (H, V) |
| + |
| +**Where:** `litestream_metered_test.go:1906-1912`. |
| + |
| +The accepted pair is now `current -> "sub/live.sqlite"`. The impostor file is still named `current -> "live.sqlite"`, a name with no separator, which is all a file name can hold. |
| + |
| +Under the thirteenth build's unprefixed encoding: |
| +- The impostor's entry `/S/current -> "live.sqlite"` does not equal the accepted `/S/current -> "sub/live.sqlite"`, whether compared cleaned or as written. |
| +- The build is refused, and the error names the impostor path without `link "`. |
| +- So all three assertions hold. The case passes with or without the fix, and the dispositions' "fails with the unprefixed encoding (mutation run)" no longer holds. |
| + |
| +**Smallest fix:** for the impostor step, accept a flat-target pair (for example `current2 -> "live2.sqlite"`, with that file present), and name the impostor `current2 -> "live2.sqlite"`. |
| + |
| +### P3-3. Owned-path quoting is untested, and the owned line is not fully quoted (AA2) |
| + |
| +**Where:** `litestream_metered.go:679`; `TestRenderQuotesEveryEntry` (`litestream_metered_test.go:1919`). |
| + |
| +- Removing `strconv.Quote(o.Path)` survives the test. |
| +- `Account`, `App` and `Host` are written raw. That is harmless today: a running fleet's routes are fixed, and a path's owner is a function of those routes and the path. Nothing else compares or parses `render()`: it is used only at `:666` and `:939`. |
| + |
| +Apart from that, the rendering is injective: |
| +- Quoted entries never begin with `owned` or equal `--`. |
| +- `Quote` round-trips. |
| + |
| +**Smallest fix:** add an owned path holding a line break to the test, or quote the owner triple as well. |
| + |
| +## Lettered questions |
| + |
| +**AA2.** `render()` is now injective over Unowned, Skipped and Problems, and over the owned paths. The raw owner triple cannot collide under fixed routes (P3-3). Nothing else consumes the rendering. |
| + |
| +**BB2.** The join is not the kernel's resolution (P3-1). |
| +- For litestream, the question does not arise: it never opens a tree link. |
| +- For tenant software, the walker and the kernel can disagree only through a link the walker does not classify, or at a linked root. |
| +- When the target cannot be looked at (anything other than ENOENT): |
| + - The problem names `p` and the target as read (`:698-699`), not the joined spelling. |
| + - ENOTDIR (a file in the middle of the target) and ELOOP (a self-link) still refuse the whole box, as at `ec55b639` (the Astra 10 class). |
| + - ENOENT under the lexical spelling is silent, and that silence is the subject of P3-1. |
| + |
| +**A–Z.** Unchanged from review fourteen, since nothing they read changed. CI's uid is still not established by committed source (X). The newline fixture needs a filesystem that allows line breaks in names. Linux tmpfs and ext4 do, and CI only cross-compiles darwin and windows. |
| + |
| +## Claims the code contradicts |
| + |
| +- **`DISPOSITIONS:268`** and the review-thirteen row: "the classification follows the kernel's resolution". It follows a lexical join (P3-1). |
| +- **`DISPOSITIONS:268`:** its Reproduction column still describes the `x/..` case. Only the bracket notes the change. |
| +- **Review-thirteen row:** "fails with the unprefixed encoding (mutation run)". No longer true at this tip (P3-2). |
| +- **`litestream_owners.go:622-623`:** "classified by where it leads (one Stat)". It is classified by the cleaned spelling. |
| +- **`litestream_metered_test.go:1897`:** the failure text still says "re-pointed through a ..". |
| +- **Review-fourteen row:** "the entry and what was looked at are one string". True of the string, but what was looked at is the cleaned string, not the entry's. |
| +- **"Package tests pass … not as root":** the author's report. Not verified here. |
| + |
| +**Verdict: accept with fixes.** Fix P3-1 (concatenate rather than `Join`, with the outside-link fixture), P3-2 and the claims. P3-3 is optional. Astra 1–4 and 8, approval H's uid binding, and the box-wide refusal decision still block wiring. |
diff --git a/internal/ops/litestream_metered.go b/internal/ops/litestream_metered.go| index 70e50c38a..2afc73cfd 100644 |
| --- a/internal/ops/litestream_metered.go |
| +++ b/internal/ops/litestream_metered.go |
| @@ -676,7 +676,7 @@ func (inv FleetInventory) render() string { |
| // one). |
| var b strings.Builder |
| for _, o := range inv.Owned { |
| - fmt.Fprintf(&b, "owned %s %s/%s %s\n", strconv.Quote(o.Path), o.Account, o.App, o.Host) |
| + fmt.Fprintf(&b, "owned %s %s %s %s\n", strconv.Quote(o.Path), strconv.Quote(o.Account), strconv.Quote(o.App), strconv.Quote(o.Host)) |
| } |
| for _, list := range [][]string{inv.Unowned, inv.Skipped, inv.Problems} { |
| sorted := append([]string(nil), list...) |
diff --git a/internal/ops/litestream_metered_test.go b/internal/ops/litestream_metered_test.go| index 7701a7448..e2c99a059 100644 |
| --- a/internal/ops/litestream_metered_test.go |
| +++ b/internal/ops/litestream_metered_test.go |
| @@ -1894,16 +1894,30 @@ func TestAcceptComparesALinkEntryAsWritten(t *testing.T) { |
| } |
| repointed := linkEntry(filepath.Join(state, "current"), "sub/./live.sqlite") |
| if _, err := MeteredFleetStore(o); !errors.Is(err, ErrUnmeterable) || !strings.Contains(err.Error(), repointed) || strings.Contains(err.Error(), entry) { |
| - t.Fatalf("the link re-pointed through a ..: %v; want the store refused naming the new pair and not the accepted one", err) |
| + t.Fatalf("the link re-pointed through a ./: %v; want the store refused naming the new pair and not the accepted one", err) |
| } |
| - // A regular database file whose own name spells the accepted pair as |
| + // A regular database file whose own name spells an accepted pair as |
| // an unprefixed encoding would (a legal file name: no separator in it) |
| // reads as a plain skipped file, never as the accepted link entry |
| - // (review thirteen, both reviewers). |
| + // (review thirteen, both reviewers). The pair accepted here has a |
| + // flat target, so the impostor's name can spell it exactly. |
| if err := os.Remove(filepath.Join(state, "current")); err != nil { |
| t.Fatal(err) |
| } |
| - impostor := filepath.Join(state, "current -> "+strconv.Quote("live.sqlite")) |
| + live2 := writeFile(t, filepath.Join(state, "live2.sqlite")) |
| + if err := os.Symlink("live2.sqlite", filepath.Join(state, "current2")); err != nil { |
| + t.Fatal(err) |
| + } |
| + o.Accept = []string{linkEntry(filepath.Join(state, "current2"), "live2.sqlite"), live, live2} |
| + if fleet, err := MeteredFleetStore(o); err != nil { |
| + t.Fatalf("the flat pair accepted: %v", err) |
| + } else { |
| + fleet.Close() |
| + } |
| + if err := os.Remove(filepath.Join(state, "current2")); err != nil { |
| + t.Fatal(err) |
| + } |
| + impostor := filepath.Join(state, "current2 -> "+strconv.Quote("live2.sqlite")) |
| if err := os.WriteFile(impostor, append([]byte("SQLite format 3\x00"), make([]byte, 100)...), 0o644); err != nil { |
| t.Fatal(err) |
| } |
| @@ -1925,4 +1939,10 @@ func TestRenderQuotesEveryEntry(t *testing.T) { |
| if !strings.Contains(a.render(), strconv.Quote("/s/a.sqlite\n/s/b.sqlite")) { |
| t.Fatalf("the entry is not quoted:\n%s", a.render()) |
| } |
| + // The owned line too, path and owner alike. |
| + c := FleetInventory{Owned: []OwnedDB{{Path: "/s/a.db\nowned \"/s/b.db\" \"x\" \"y\" \"z\"", Account: "x", App: "y", Host: "z"}}} |
| + d := FleetInventory{Owned: []OwnedDB{{Path: "/s/a.db", Account: "x", App: "y", Host: "z"}, {Path: "/s/b.db", Account: "x", App: "y", Host: "z"}}} |
| + if c.render() == d.render() || !strings.Contains(c.render(), strconv.Quote(c.Owned[0].Path)) { |
| + t.Fatalf("an owned path with a line break is not quoted:\n%s", c.render()) |
| + } |
| } |
diff --git a/internal/ops/litestream_owners.go b/internal/ops/litestream_owners.go| index fa8b1b28f..ab92a1a14 100644 |
| --- a/internal/ops/litestream_owners.go |
| +++ b/internal/ops/litestream_owners.go |
| @@ -668,8 +668,9 @@ func strictTreeDBs(root string) (dbs, skipped, problems []string) { |
| } |
| switch { |
| case fi.Mode()&os.ModeSymlink != 0: |
| - // Classified by where it leads (Stat follows it for that |
| - // and nothing else: it is never walked into or opened). |
| + // Classified by where its target's spelling leads (one Stat |
| + // of that spelling, as the kernel resolves it; the link is |
| + // never walked into or opened). |
| // A link entry is named with its path and target, each |
| // quoted behind a word no path begins with, so a link |
| // re-pointed after it was accepted reads as a change and |
| @@ -687,9 +688,13 @@ func strictTreeDBs(root string) (dbs, skipped, problems []string) { |
| // directory when relative), never by reading the link |
| // again: what the entry says and what was looked at are one |
| // string, whatever the link is re-pointed to meanwhile. |
| + // The spelling is concatenated, never cleaned: the kernel |
| + // resolves a ".." in the target physically (through whatever |
| + // the component before it led to), and a cleaned join would |
| + // look somewhere else. |
| spelled := dest |
| if !filepath.IsAbs(spelled) { |
| - spelled = filepath.Join(dir, spelled) |
| + spelled = dir + string(filepath.Separator) + spelled |
| } |
| target, terr := os.Stat(spelled) |
| // A problem names the path first (nothing compares a |
diff --git a/internal/ops/litestream_owners_test.go b/internal/ops/litestream_owners_test.go| index d6e3cfc72..440063bb5 100644 |
| --- a/internal/ops/litestream_owners_test.go |
| +++ b/internal/ops/litestream_owners_test.go |
| @@ -711,6 +711,29 @@ func TestStrictTreeNamesALinkToAFile(t *testing.T) { |
| if !contains(skipped, linkEntry(filepath.Join(root, "newline"), broken)) || strings.Contains(strings.Join(skipped, "|"), "\n") { |
| t.Fatalf("a target with a line break: skipped %q; want it quoted", skipped) |
| } |
| + // A relative target with a ".." after a component that is itself a |
| + // link to a directory outside the tree: the kernel steps up from where |
| + // that link led, so the spelling is resolved as concatenated, never |
| + // cleaned (a cleaned join would look beside the link and find |
| + // nothing). (Review fifteen, Opus P3-1.) |
| + outside := platformDir(t) |
| + mkdirs(t, filepath.Join(outside, "o"), filepath.Join(outside, "hidden", "sub"), filepath.Join(outside, "hidden", "dbs")) |
| + if err := os.Symlink(filepath.Join(outside, "hidden", "sub"), filepath.Join(outside, "o", "lnk")); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := os.Symlink("../"+filepath.Base(outside)+"/o/lnk/../dbs", filepath.Join(root, "stepup")); err != nil { |
| + t.Fatal(err) |
| + } |
| + if real, err := filepath.EvalSymlinks(filepath.Join(root, "stepup")); err != nil || real != filepath.Join(outside, "hidden", "dbs") { |
| + t.Fatalf("fixture: the kernel resolves the link to %q %v; want the hidden directory", real, err) |
| + } |
| + _, _, problems = strictTreeDBs(root) |
| + if !containsPrefix(problems, filepath.Join(root, "stepup")) { |
| + t.Fatalf("a link whose target steps up through an outside link to a directory: problems %v; want it said", problems) |
| + } |
| + if err := os.Remove(filepath.Join(root, "stepup")); err != nil { |
| + t.Fatal(err) |
| + } |
| // A link whose target cannot be looked at is a problem, not nothing. |
| if os.Geteuid() != 0 { |
| locked := filepath.Join(platformDir(t), "locked") |