carlos / platform Public

dbdoor: PlanRestore refuses a header that does not describe its file (Plan 5 Task 2 review)

The last planned file's LTX header comes from the replica, and ltx's
UnmarshalBinary checks only the magic: Commit times PageSize could wrap
negative and slip under the seed size cap. PlanRestore now refuses,
as its own error, a page size SQLite does not allow, a zero commit, or
TXIDs other than the listed file's; a valid page size bounds the product
below 2^48.

Review minors taken: tests size a plan over a snapshot chain, a target at
an L0 end after the snapshot, a shrinking (VACUUM) database, and LTXBytes
as the listing's sizes summed; the RestoreToTarget comment points at
plan.TXID.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 2 weeks ago 10c960abe28843858636ae99b5e3579c1b654900 parent 5f575f5

3 files changed, +146 −1

internal/dbdoor/plan.go +9 −0
internal/dbdoor/plan_test.go +135 −0
internal/dbdoor/restore.go +2 −1

CI cancelled — superseded by a newer push — run details

diff --git a/internal/dbdoor/plan.go b/internal/dbdoor/plan.go

index c5632bc31..826a24c63 100644

--- a/internal/dbdoor/plan.go

+++ b/internal/dbdoor/plan.go

@@ -77,6 +77,15 @@ func PlanRestore(ctx context.Context, c *Client, target uint64) (RestorePlan, er

if err := h.UnmarshalBinary(buf); err != nil {

return RestorePlan{}, fmt.Errorf("dbdoor: the last planned file's header: %w", err)

}

+ // The header is the replica's word, so untrusted, and UnmarshalBinary

+ // checks only its magic. A page size SQLite allows (512 to 65536)

+ // bounds the product below 2^48, so it cannot wrap; a zero commit is

+ // no database; and TXIDs other than the listed file's mean the header

+ // describes some other file.

+ if !ltx.IsValidPageSize(h.PageSize) || h.Commit == 0 || h.MinTXID != last.MinTXID || h.MaxTXID != last.MaxTXID {

+ return RestorePlan{}, fmt.Errorf("dbdoor: the last planned file's header does not describe it (level %d %s: page size %d, commit %d, txids %s)",

+ last.Level, Span(uint64(last.MinTXID), uint64(last.MaxTXID)), h.PageSize, h.Commit, Span(uint64(h.MinTXID), uint64(h.MaxTXID)))

+ }

p.RestoredBytes = int64(h.Commit) * int64(h.PageSize)

return p, nil

}

diff --git a/internal/dbdoor/plan_test.go b/internal/dbdoor/plan_test.go

index 6677fd6c4..151169bd2 100644

--- a/internal/dbdoor/plan_test.go

+++ b/internal/dbdoor/plan_test.go

@@ -6,6 +6,8 @@ import (

"errors"

"fmt"

"io"

+ "net/http"

+ "net/http/httptest"

"os"

"strings"

"testing"

@@ -36,6 +38,16 @@ func TestPlanRestoreSizesTheRestoreExactly(t *testing.T) {

if plan.RestoredBytes != st.Size() || plan.TXID != r.TXID || plan.LTXBytes <= 0 || plan.Files == 0 || plan.Target != 0 {

t.Fatalf("plan %+v, restored %d bytes at %d; want the plan's size and txid", plan, st.Size(), r.TXID)

}

+ // With no snapshot the plan is every L0 file, so LTXBytes is the

+ // listing's sizes summed.

+ var listed int64

+ files := collect(t, NewClient(newTestDoor(t, src).do), 0, 0)

+ for _, f := range files {

+ listed += f.Size

+ }

+ if plan.Files != len(files) || plan.LTXBytes != listed {

+ t.Fatalf("plan %+v, want %d files of %d bytes", plan, len(files), listed)

+ }

}

func TestRestoreToATargetTXID(t *testing.T) {

@@ -147,6 +159,25 @@ func TestPlanRestoreTargetBeforeHistory(t *testing.T) {

if rows := countRows(t, r.Path); rows != 4*dbdoortest.Rows {

t.Fatalf("rows = %d, want every batch's %d", rows, 4*dbdoortest.Rows)

}

+ if st, err := os.Stat(r.Path); err != nil || st.Size() != latest.RestoredBytes {

+ t.Fatalf("restored over the snapshot: stat %v, err %v; want the planned %d bytes", st, err, latest.RestoredBytes)

+ }

+ // A target at an L0 file's end after the snapshot: the snapshot plus

+ // that one file.

+ mid := uint64(files[2].MaxTXID)

+ atMid, err := PlanRestore(context.Background(), NewClient(newTestDoor(t, src).do), mid)

+ if err != nil || atMid.TXID != mid || atMid.Files != 2 || atMid.Target != mid {

+ t.Fatalf("a target after the snapshot: plan %+v, err %v; want the snapshot and one L0 file ending at %d", atMid, err, mid)

+ }

+ rm, err := RestoreToTarget(context.Background(), NewClient(newTestDoor(t, src).do), t.TempDir(), atMid.TXID)

+ if err != nil {

+ t.Fatal(err)

+ }

+ defer os.RemoveAll(rm.Dir)

+ if st, err := os.Stat(rm.Path); err != nil || st.Size() != atMid.RestoredBytes || countRows(t, rm.Path) != 3*dbdoortest.Rows {

+ t.Fatalf("restored at %d: stat %v, err %v; want %d bytes and three batches", mid, st, err, atMid.RestoredBytes)

+ }

atSnap, err := PlanRestore(context.Background(), NewClient(newTestDoor(t, src).do), uint64(snap.MaxTXID))

if err != nil || atSnap.TXID != uint64(snap.MaxTXID) || atSnap.Files != 1 || atSnap.Target != uint64(snap.MaxTXID) {

@@ -219,3 +250,107 @@ func grow(t *testing.T, path string) {

}

+// The header PlanRestore sizes the restore from comes from the replica,

+// so it is untrusted. A page size SQLite cannot have, a zero commit, or

+// TXIDs that are not the listed file's are refused rather than sized; an

+// unchecked Commit times PageSize can wrap negative and slip under any

+// size cap.

+func TestPlanRestoreRefusesAHeaderThatDoesNotDescribeItsFile(t *testing.T) {

+ src := t.TempDir()

+ dbdoortest.Replicate(t, src, 2)

+ files := collect(t, NewClient(newTestDoor(t, src).do), 0, 0)

+ last := files[len(files)-1]

+ rc, err := lsfile.NewReplicaClient(src).OpenLTXFile(context.Background(), 0, last.MinTXID, last.MaxTXID, 0, ltx.HeaderSize)

+ if err != nil {

+ t.Fatal(err)

+ }

+ honest := make([]byte, ltx.HeaderSize)

+ _, err = io.ReadFull(rc, honest)

+ rc.Close()

+ if err != nil {

+ t.Fatal(err)

+ }

+ for _, tc := range []struct {

+ name string

+ edit func(h *ltx.Header)

+ }{

+ {"a page size that wraps the size", func(h *ltx.Header) { h.PageSize, h.Commit = 0xFFFFFFFF, 0xFFFFFFFF }},

+ {"a page size that is not a power of two", func(h *ltx.Header) { h.PageSize = 4000 }},

+ {"a zero page size", func(h *ltx.Header) { h.PageSize = 0 }},

+ {"a zero commit", func(h *ltx.Header) { h.Commit = 0 }},

+ {"another file's max txid", func(h *ltx.Header) { h.MaxTXID++ }},

+ {"another file's min txid", func(h *ltx.Header) { h.MinTXID++ }},

+ } {

+ t.Run(tc.name, func(t *testing.T) {

+ var h ltx.Header

+ if err := h.UnmarshalBinary(honest); err != nil {

+ t.Fatal(err)

+ }

+ tc.edit(&h)

+ crafted, _ := h.MarshalBinary()

+ door := newTestDoor(t, src)

+ headerTail := GetTail(0, uint64(last.MinTXID), uint64(last.MaxTXID))

+ do := func(ctx context.Context, tail string, hd http.Header) (*http.Response, error) {

+ if tail == headerTail && hd.Get("Range") == RangeHeader(0, ltx.HeaderSize) {

+ rec := httptest.NewRecorder()

+ rec.WriteHeader(http.StatusPartialContent)

+ rec.Write(crafted)

+ return rec.Result(), nil

+ }

+ return door.do(ctx, tail, hd)

+ }

+ plan, err := PlanRestore(context.Background(), NewClient(do), 0)

+ if err == nil || errors.Is(err, ErrNothingReplicated) || errors.Is(err, ErrHistoryGone) {

+ t.Fatalf("plan %+v, err = %v; want the header refused as its own error", plan, err)

+ }

+ if plan != (RestorePlan{}) {

+ t.Fatalf("a refused header still planned %+v", plan)

+ }

+ })

+ }

+// A database that shrinks (rows deleted, then VACUUM) is sized by its last

+// file's commit, not by the largest the history ever held.

+func TestPlanRestoreFollowsAShrinkingDatabase(t *testing.T) {

+ dir := t.TempDir()

+ db := dbdoortest.Replicate(t, dir, 1)

+ grow(t, db)

+ dbdoortest.Sync(t, db, dir)

+ big, err := PlanRestore(context.Background(), NewClient(newTestDoor(t, dir).do), 0)

+ if err != nil {

+ t.Fatal(err)

+ }

+ sdb, err := sql.Open("sqlite", db)

+ if err != nil {

+ t.Fatal(err)

+ }

+ if _, err := sdb.Exec(`DELETE FROM t WHERE body LIKE 'grow-%'; VACUUM`); err != nil {

+ sdb.Close()

+ t.Fatal(err)

+ }

+ sdb.Close()

+ dbdoortest.Sync(t, db, dir)

+ small, err := PlanRestore(context.Background(), NewClient(newTestDoor(t, dir).do), 0)

+ if err != nil {

+ t.Fatal(err)

+ }

+ if small.TXID <= big.TXID || small.RestoredBytes >= big.RestoredBytes {

+ t.Fatalf("after the shrink: planned %+v, before %+v; want a later, smaller plan", small, big)

+ }

+ r, err := RestoreToTarget(context.Background(), NewClient(newTestDoor(t, dir).do), t.TempDir(), small.TXID)

+ if err != nil {

+ t.Fatal(err)

+ }

+ defer os.RemoveAll(r.Dir)

+ st, err := os.Stat(r.Path)

+ if err != nil {

+ t.Fatal(err)

+ }

+ if st.Size() != small.RestoredBytes {

+ t.Fatalf("restored %d bytes, planned %d", st.Size(), small.RestoredBytes)

+ }

diff --git a/internal/dbdoor/restore.go b/internal/dbdoor/restore.go

index caf2f7493..ecb6edc92 100644

--- a/internal/dbdoor/restore.go

+++ b/internal/dbdoor/restore.go

@@ -76,7 +76,8 @@ func RestoreTo(ctx context.Context, c *Client, parent string) (*Restored, error)

// copy never reaches past it and Restored.TXID is target. A target no

// retained chain ends at (older than the retained history, inside a

// compacted file, or past the latest transaction) is refused as

-// ErrHistoryGone. PlanRestore says which target to ask for.

+// ErrHistoryGone. Ask for a PlanRestore's TXID: that restore produces

+// exactly the plan's RestoredBytes.

func RestoreToTarget(ctx context.Context, c *Client, parent string, target uint64) (*Restored, error) {

dir, err := os.MkdirTemp(parent, "carlos-db-")

if err != nil {