amadan
Route VLOOKUP/INDEX/MATCH through the range cap
fnVLookup, fnIndex, fnMatch (fn_lookup.go) iterated a Range's raw r0..r1 bounds via env.CellValue directly, never consulting maxRangeCells the way evalCallArg and evalAggArgs do. Two consequences: a pathological reference (decodeRef accepts any int row) let a formula like VLOOKUP(1,A1:A<huge>,1) iterate synchronously forever, the same wedged- process failure mode the cycle-detection tests guard against, reached through a different door; and because collectRefs also caps at maxRangeCells, an over-cap range's cells were left untracked as precedents while the lookup family kept reading them directly, breaking the "untracked range always resolves to #REF!" invariant and opening a stale-cache hole with no recovery signal. Move rangeBounds into eval.go and factor the cap check out of rangeCells into rangeCellCount, shared by rangeCells and the lookup family's new boundedRangeBounds, so the cap lives in one place instead of three copies that could drift. rangeCellCount also checks each span against maxRangeCells before multiplying them, so a pathological reference whose row or column span is itself near the int range's limit cannot overflow the product and slip past the cap. Tests (TDD: written first, watched fail — three hung, INDEX returned a stale value instead of erroring): a range far past the cap now resolves to #REF! promptly for VLOOKUP, INDEX, MATCH(0) and MATCH(1), each under the same 2-second timeout the cycle-detection tests use so a regression fails instead of wedging the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
3 files changed,
+125
−29
internal/sheets/calc/eval.go+46 −14internal/sheets/calc/fn_lookup.go+30 −15internal/sheets/calc/fn_lookup_test.go+49 −0
Intent
diff --git a/internal/sheets/calc/eval.go b/internal/sheets/calc/eval.go| index 291d0a8..9e71251 100644 |
| --- a/internal/sheets/calc/eval.go |
| +++ b/internal/sheets/calc/eval.go |
| @@ -83,26 +83,58 @@ func refKey(r *Ref) CellKey { |
| // Recalc's MaxDirtyPerPass, which caps the symmetric fan-out case. |
| const maxRangeCells = 200_000 |
| -// rangeCells expands a Range into every CellKey it covers, normalizing a |
| -// range written back-to-front (e.g. "A5:A1"). Used both to fetch a |
| -// function call argument's values and to build the dependency graph's |
| -// precedent edges for a formula that reads a range. |
| -// |
| -// ok is false, with cells nil, when the range would exceed maxRangeCells — |
| -// nothing is materialized in that case, mirroring Recalc's own cap: the |
| -// caller decides how to report it rather than rangeCells silently |
| -// truncating the range to some prefix of itself. |
| -func rangeCells(rng *Range) (cells []CellKey, ok bool) { |
| - r0, r1 := rng.From.RowPos, rng.To.RowPos |
| - c0, c1 := rng.From.ColPos, rng.To.ColPos |
| +// rangeBounds normalizes a *Range's positions, handling a range written |
| +// back-to-front (e.g. "A5:A1"). Shared by rangeCells below, which also |
| +// materializes every cell, and by the lookup family (fn_lookup.go's |
| +// boundedRangeBounds), which only need the four bounds to compute an |
| +// offset or walk cells directly against env rather than through a |
| +// flattened []CellKey — see fn_lookup.go's package doc comment for why |
| +// VLOOKUP/INDEX/MATCH need the range's shape, not evalCallArg's flattened |
| +// form. |
| +func rangeBounds(rng *Range) (r0, r1, c0, c1 int) { |
| + r0, r1 = rng.From.RowPos, rng.To.RowPos |
| + c0, c1 = rng.From.ColPos, rng.To.ColPos |
| if r0 > r1 { |
| r0, r1 = r1, r0 |
| } |
| if c0 > c1 { |
| c0, c1 = c1, c0 |
| } |
| - count := (r1 - r0 + 1) * (c1 - c0 + 1) |
| - if count > maxRangeCells { |
| + return |
| +} |
| + |
| +// rangeCellCount reports how many cells a normalized range bounds covers, |
| +// and whether that count is within maxRangeCells. decodeRef (token.go) |
| +// accepts any int row or column, so a written reference can put r1-r0+1 |
| +// or c1-c0+1 itself near the int range's own limit; multiplying two such |
| +// spans together before checking them would overflow and could wrap |
| +// around to a small or negative product, silently passing a pathological |
| +// range through the cap it exists to enforce. So each span is checked |
| +// against maxRangeCells individually — it alone already caps span sizes |
| +// well below where their product could overflow — before they are ever |
| +// multiplied. |
| +func rangeCellCount(r0, r1, c0, c1 int) (count int, ok bool) { |
| + rows := r1 - r0 + 1 |
| + cols := c1 - c0 + 1 |
| + if rows > maxRangeCells || cols > maxRangeCells { |
| + return 0, false |
| + } |
| + count = rows * cols |
| + return count, count <= maxRangeCells |
| +} |
| + |
| +// rangeCells expands a Range into every CellKey it covers. Used both to |
| +// fetch a function call argument's values and to build the dependency |
| +// graph's precedent edges for a formula that reads a range. |
| +// |
| +// ok is false, with cells nil, when the range would exceed maxRangeCells — |
| +// nothing is materialized in that case, mirroring Recalc's own cap: the |
| +// caller decides how to report it rather than rangeCells silently |
| +// truncating the range to some prefix of itself. |
| +func rangeCells(rng *Range) (cells []CellKey, ok bool) { |
| + r0, r1, c0, c1 := rangeBounds(rng) |
| + count, ok := rangeCellCount(r0, r1, c0, c1) |
| + if !ok { |
| return nil, false |
| } |
| out := make([]CellKey, 0, count) |
diff --git a/internal/sheets/calc/fn_lookup.go b/internal/sheets/calc/fn_lookup.go| index 6644a16..5050863 100644 |
| --- a/internal/sheets/calc/fn_lookup.go |
| +++ b/internal/sheets/calc/fn_lookup.go |
| @@ -14,18 +14,24 @@ func init() { |
| registerNode("MATCH", fnMatch) |
| } |
| -// rangeBounds normalizes a *Range's positions the same way rangeCells |
| -// does (eval.go), without materializing every cell — these functions |
| -// only need the four bounds to compute an offset, not a flattened slice. |
| -func rangeBounds(rng *Range) (r0, r1, c0, c1 int) { |
| - r0, r1 = rng.From.RowPos, rng.To.RowPos |
| - c0, c1 = rng.From.ColPos, rng.To.ColPos |
| - if r0 > r1 { |
| - r0, r1 = r1, r0 |
| - } |
| - if c0 > c1 { |
| - c0, c1 = c1, c0 |
| - } |
| +// boundedRangeBounds is rangeBounds (eval.go) plus the same cap |
| +// rangeCells, evalCallArg and evalAggArgs all enforce on a Range |
| +// (maxRangeCells; see eval.go's rangeCellCount). VLOOKUP, INDEX and MATCH |
| +// evaluate their range argument's shape directly against env rather than |
| +// through rangeCells — they need the range's four bounds to compute an |
| +// offset or walk cells directly, not a flattened []CellKey (see this |
| +// file's package doc comment) — so without this check they would be the |
| +// one path into a Range that never consults the cap at all: unbounded |
| +// synchronous iteration over a pathological reference (decodeRef accepts |
| +// any int row), and — because collectRefs also uses rangeCells and so |
| +// never wires an over-cap range's cells as precedents — a stale-cache |
| +// hole where these functions could read live cells the graph never |
| +// tracked. ok is false when the range is over the cap; callers must |
| +// resolve to #REF! rather than iterate, the same answer evalCallArg gives |
| +// an ordinary function for the same over-cap range. |
| +func boundedRangeBounds(rng *Range) (r0, r1, c0, c1 int, ok bool) { |
| + r0, r1, c0, c1 = rangeBounds(rng) |
| + _, ok = rangeCellCount(r0, r1, c0, c1) |
| return |
| } |
| @@ -74,7 +80,10 @@ func fnVLookup(argNodes []Node, env Env) Value { |
| approx = b |
| } |
| - r0, r1, c0, c1 := rangeBounds(rng) |
| + r0, r1, c0, c1, withinCap := boundedRangeBounds(rng) |
| + if !withinCap { |
| + return errValue(ErrRef) |
| + } |
| cols := c1 - c0 + 1 |
| if ci < 1 || ci > cols { |
| return errValue(ErrRef) |
| @@ -147,7 +156,10 @@ func fnIndex(argNodes []Node, env Env) Value { |
| } |
| return Eval(argNodes[0], env) |
| } |
| - r0, r1, c0, c1 := rangeBounds(rng) |
| + r0, r1, c0, c1, withinCap := boundedRangeBounds(rng) |
| + if !withinCap { |
| + return errValue(ErrRef) |
| + } |
| if r0+rowN-1 > r1 || c0+colN-1 > c1 { |
| return errValue(ErrRef) |
| } |
| @@ -185,7 +197,10 @@ func fnMatch(argNodes []Node, env Env) Value { |
| matchType = int(mtF) |
| } |
| - r0, r1, c0, c1 := rangeBounds(rng) |
| + r0, r1, c0, c1, withinCap := boundedRangeBounds(rng) |
| + if !withinCap { |
| + return errValue(ErrRef) |
| + } |
| if r0 != r1 && c0 != c1 { |
| return errValue(ErrNA) |
| } |
diff --git a/internal/sheets/calc/fn_lookup_test.go b/internal/sheets/calc/fn_lookup_test.go| index 17dea7e..848ce6e 100644 |
| --- a/internal/sheets/calc/fn_lookup_test.go |
| +++ b/internal/sheets/calc/fn_lookup_test.go |
| @@ -107,3 +107,52 @@ func TestMatchOmittedMatchTypeDefaultsToNearestAscending(t *testing.T) { |
| t.Fatalf("MATCH with match_type omitted = %v, want 2 (nearest-ascending by default)", v) |
| } |
| } |
| + |
| +// pathologicalRange is a range far past maxRangeCells — decodeRef accepts |
| +// any int row, so nothing upstream stops a formula from naming one. The |
| +// lookup family must reject it as #REF! the same way evalCallArg and |
| +// evalAggArgs do for every other function, not iterate r0..r1 raw: that |
| +// raw iteration is both an unbounded-hang risk (the failure mode the |
| +// cycle-detection tests exist to prevent, reached through a different |
| +// door) and a stale-cache hole (collectRefs leaves an over-cap range |
| +// untracked as a precedent, so a formula that reads it directly reads |
| +// cells the graph never wired and won't be dirtied when they change). |
| +func pathologicalRange() *Range { |
| + return &Range{From: &Ref{RowPos: 0, ColPos: 0}, To: &Ref{RowPos: 1 << 40, ColPos: 0}} |
| +} |
| + |
| +// TestVLookupOverCapRangeIsRefNotHang must return #REF! promptly rather |
| +// than hang. It carries the same 2-second timeout the cycle-detection |
| +// tests use (graph_cycle_test.go's evalWithTimeout) so a regression here |
| +// fails the test instead of wedging the whole test run. |
| +func TestVLookupOverCapRangeIsRefNotHang(t *testing.T) { |
| + call := &Call{Name: "VLOOKUP", Args: []Node{&Num{V: 1}, pathologicalRange(), &Num{V: 1}, &Bool{V: false}}} |
| + v := evalWithTimeout(t, func() Value { return Eval(call, emptyEnv{}) }) |
| + if v.Kind != KindErr || v.Err != ErrRef { |
| + t.Fatalf("VLOOKUP over-cap range = %v, want #REF!", v) |
| + } |
| +} |
| + |
| +func TestIndexOverCapRangeIsRefNotHang(t *testing.T) { |
| + call := &Call{Name: "INDEX", Args: []Node{pathologicalRange(), &Num{V: 1}, &Num{V: 1}}} |
| + v := evalWithTimeout(t, func() Value { return Eval(call, emptyEnv{}) }) |
| + if v.Kind != KindErr || v.Err != ErrRef { |
| + t.Fatalf("INDEX over-cap range = %v, want #REF!", v) |
| + } |
| +} |
| + |
| +func TestMatchExactOverCapRangeIsRefNotHang(t *testing.T) { |
| + call := &Call{Name: "MATCH", Args: []Node{&Num{V: 1}, pathologicalRange(), &Num{V: 0}}} |
| + v := evalWithTimeout(t, func() Value { return Eval(call, emptyEnv{}) }) |
| + if v.Kind != KindErr || v.Err != ErrRef { |
| + t.Fatalf("MATCH(match_type=0) over-cap range = %v, want #REF!", v) |
| + } |
| +} |
| + |
| +func TestMatchNearestOverCapRangeIsRefNotHang(t *testing.T) { |
| + call := &Call{Name: "MATCH", Args: []Node{&Num{V: 1}, pathologicalRange(), &Num{V: 1}}} |
| + v := evalWithTimeout(t, func() Value { return Eval(call, emptyEnv{}) }) |
| + if v.Kind != KindErr || v.Err != ErrRef { |
| + t.Fatalf("MATCH(match_type=1) over-cap range = %v, want #REF!", v) |
| + } |
| +} |