diff --git a/.kiro/specs/calculator/design.md b/.kiro/specs/calculator/design.md index b6d4262..8368b17 100644 --- a/.kiro/specs/calculator/design.md +++ b/.kiro/specs/calculator/design.md @@ -577,6 +577,41 @@ the demotion policy at the denominator cap, rational-to-decimal rendering and rounding, and a regression test per bug in 2.7.1 - including one asserting `9007199254740993` round-trips, which nothing currently guards. +##### Allocation-failure testing + +`errdefer` cleanup paths are not reachable by ordinary tests, and covering them +is worthless on its own: the line exists to *prevent a leak*, so executing it +proves nothing unless the test also asserts that nothing leaked. + +`std.testing.FailingAllocator` fails after exactly N allocations. Sweeping N +across an operation's whole allocation sequence, with `testing.allocator` +underneath to detect leaks at test end, turns those paths into real assertions. +(`std.mem.Allocator.failing` fails on the *first* allocation, which only covers +entry-point OOM, not the interesting mid-construction failures.) + +This is not a coverage exercise; the sweep immediately found three genuine bugs +in `rational.zig` that ordinary tests could never reach: + +1. **A double free.** `initOwned` took its numerator and denominator *by value* + and freed them via `errdefer` on its own failure, while every caller ALSO had + an `errdefer` for the same values. Any allocation failure inside normalization + freed them twice, which segfaulted. +2. **A dangling copy.** Because the parts were passed by value, and normalization + can reallocate limbs, a caller's `errdefer` could free a stale pointer even + without the double free. + Fixed by renaming to `finish` and taking both parts **by pointer**, with the + contract that ownership transfers only on success: on failure the caller's + `errdefer` sees live, current values and frees them exactly once. +3. **Two struct-literal leaks.** `initInt`, `initZero` and `initRatio` built + `.{ .num = try initSet(...), .den = try initSet(...) }` in a single literal. + An `errdefer` cannot cover a value that does not exist yet, so a failure on + the second allocation orphaned the first. Fixed by building the parts as + separate statements, each with its own `errdefer`. `floor` and `factorial` + were also missing an `errdefer` on their denominator. + +The remaining uncovered lines in these modules are `unreachable` arms guarded +upstream, and diagnostic paths inside the tests themselves. + --- ## 3. Struct Layout Engine diff --git a/.kiro/specs/calculator/tasks.md b/.kiro/specs/calculator/tasks.md index e471603..cefe496 100644 --- a/.kiro/specs/calculator/tasks.md +++ b/.kiro/specs/calculator/tasks.md @@ -167,9 +167,17 @@ behavioral change (rationale in design.md 2.7.9): expected value shifts with optimize mode verifies nothing, so the `mod` test now asserts the values the definition requires instead of deriving them from `@mod`. Worth reporting upstream. +- Allocation-failure sweeps added using `std.testing.FailingAllocator` + (`fail_index` = fail after N allocations) over `testing.allocator`, so leaks are + detected rather than the cleanup lines merely being executed. These found three + real bugs unreachable by ordinary tests: a double free (`initOwned` freed values + its callers also freed), a potential dangling copy (parts passed by value while + normalization can reallocate limbs), and struct-literal leaks in `initInt` / + `initZero` / `initRatio` where an `errdefer` cannot cover a value that does not + exist yet. `floor` and `factorial` were also missing a denominator `errdefer`. + See design.md 2.7.10. Uncovered lines in the numeric modules went 25 -> 8. -#### 2.0c: `evalString` and the display path move to `Number` -- Mechanical signature churn through evaluator/formatter/CLI/TUI tests. This is +#### 2.0c: `evalString` and the display path move to `Number`- Mechanical signature churn through evaluator/formatter/CLI/TUI tests. This is the commit where `main.zig`'s output assertions get reviewed. - Lift the limits this removes: `formatter.is_integer`'s `< 2^53` gate and `evaluator.factorial`'s `x > 170` rejection (also the source of the misleading diff --git a/engine/src/number.zig b/engine/src/number.zig index e823183..a23ab8b 100644 --- a/engine/src/number.zig +++ b/engine/src/number.zig @@ -858,3 +858,112 @@ test "max and min with an inexact operand return that operand as-is" { try testing.expect(!hi.isExact()); try testing.expectEqual(@as(f64, 2.0), hi.toFloat(alloc)); } + +// -- Allocation-failure safety -- +// +// Mirrors the sweep in rational.zig: fail the Nth allocation for every N, and +// let testing.allocator's leak detection verify that partially-built values are +// released. This is what actually validates the cleanup paths; merely executing +// them proves nothing. + +fn oomSweep(comptime body: fn (Allocator) anyerror!void) !void { + var fail_index: usize = 0; + while (fail_index < 512) : (fail_index += 1) { + var failing = std.testing.FailingAllocator.init(alloc, .{ .fail_index = fail_index }); + if (body(failing.allocator())) |_| { + return; + } else |err| { + if (err != error.OutOfMemory) return err; + } + } + return error.OomSweepNeverCompleted; +} + +fn bodyExactArithmetic(a: Allocator) anyerror!void { + var x = try Number.parse(a, "0.1"); + defer x.deinit(); + var y = try Number.parse(a, "0.2"); + defer y.deinit(); + + var sum = try Number.add(a, x, y); + defer sum.deinit(); + var diff = try Number.sub(a, x, y); + defer diff.deinit(); + var prod = try Number.mul(a, x, y); + defer prod.deinit(); + var quot = try Number.div(a, x, y); + defer quot.deinit(); + var neg = try Number.negate(a, x); + defer neg.deinit(); + var magnitude = try Number.abs(a, neg); + defer magnitude.deinit(); + _ = try Number.order(a, x, y); +} + +fn bodyPowSqrtFactorial(a: Allocator) anyerror!void { + var base = try Number.fromInt(a, 4); + defer base.deinit(); + var exp = try Number.fromInt(a, 3); + defer exp.deinit(); + + var p = try Number.pow(a, base, exp); + defer p.deinit(); + var r = try Number.sqrt(a, base); + defer r.deinit(); + if (try Number.factorial(a, exp)) |f| { + var value = f; + value.deinit(); + } +} + +fn bodyRoundingAndSelection(a: Allocator) anyerror!void { + var x = try Number.parse(a, "-3.75"); + defer x.deinit(); + var y = try Number.fromInt(a, 2); + defer y.deinit(); + + var f = try Number.floor(a, x); + defer f.deinit(); + var c = try Number.ceil(a, x); + defer c.deinit(); + var rounded = try Number.round(a, x); + defer rounded.deinit(); + var m = try Number.mod(a, x, y); + defer m.deinit(); + var hi = try Number.max(a, x, y); + defer hi.deinit(); + var lo = try Number.min(a, x, y); + defer lo.deinit(); + var copy = try x.clone(); + defer copy.deinit(); +} + +fn bodyRendering(a: Allocator) anyerror!void { + var x = try Number.parse(a, "0.1"); + defer x.deinit(); + var three = try Number.fromInt(a, 3); + defer three.deinit(); + var third = try Number.div(a, x, three); + defer third.deinit(); + + const d = try third.toDecimalString(a, 12); + a.free(d.text); + if (try third.toFractionString(a)) |frac| a.free(frac); + _ = third.toFloat(a); +} + +test "OOM safety: exact arithmetic" { + try oomSweep(bodyExactArithmetic); +} + +test "OOM safety: pow, sqrt and factorial" { + try oomSweep(bodyPowSqrtFactorial); +} + +test "OOM safety: rounding, mod, max/min and clone" { + try oomSweep(bodyRoundingAndSelection); +} + +test "OOM safety: rendering" { + try oomSweep(bodyRendering); +} diff --git a/engine/src/rational.zig b/engine/src/rational.zig index 42b27a3..79054c3 100644 --- a/engine/src/rational.zig +++ b/engine/src/rational.zig @@ -39,39 +39,44 @@ pub const Rational = struct { /// Zero (`0/1`). pub fn initZero(allocator: Allocator) Error!Rational { - return .{ - .num = try Managed.initSet(allocator, 0), - .den = try Managed.initSet(allocator, 1), - }; + return initInt(allocator, 0); } /// An integer value (`value/1`). pub fn initInt(allocator: Allocator, value: anytype) Error!Rational { - return .{ - .num = try Managed.initSet(allocator, value), - .den = try Managed.initSet(allocator, 1), - }; + // Built separately rather than inside a struct literal: if the second + // allocation fails, the first must still be released. + var num = try Managed.initSet(allocator, value); + errdefer num.deinit(); + const den = try Managed.initSet(allocator, 1); + return .{ .num = num, .den = den }; } /// A ratio, reduced on construction. Errors if `den` is zero. pub fn initRatio(allocator: Allocator, num: anytype, den: anytype) Error!Rational { - var result: Rational = .{ - .num = try Managed.initSet(allocator, num), - .den = try Managed.initSet(allocator, den), - }; - errdefer result.deinit(); - if (result.den.eqlZero()) return Error.DivisionByZero; - try result.normalize(allocator); - return result; + // Built separately rather than inside a struct literal: if the second + // allocation fails, the first must still be released, and an errdefer + // cannot cover a value that does not exist yet. + var n = try Managed.initSet(allocator, num); + errdefer n.deinit(); + var d = try Managed.initSet(allocator, den); + errdefer d.deinit(); + return finish(allocator, &n, &d); } - /// Take ownership of two `Managed` values, normalizing them. - fn initOwned(allocator: Allocator, num: Managed, den: Managed) Error!Rational { - var result: Rational = .{ .num = num, .den = den }; - errdefer result.deinit(); - if (result.den.eqlZero()) return Error.DivisionByZero; - try result.normalize(allocator); - return result; + /// Assemble a reduced `Rational` from a numerator and denominator. + /// + /// Ownership transfers ONLY on success. Both parts are taken by pointer and + /// normalized in place, so on failure the caller's `errdefer` still sees + /// live, current values and frees them exactly once. + /// + /// Taking them by value would be unsound twice over: the caller's errdefer + /// and this function would both free them (a double free), and normalization + /// can reallocate limbs, which would leave the caller's copy dangling. + fn finish(allocator: Allocator, num: *Managed, den: *Managed) Error!Rational { + if (den.eqlZero()) return Error.DivisionByZero; + try normalizeParts(allocator, num, den); + return .{ .num = num.*, .den = den.* }; } pub fn deinit(self: *Rational) void { @@ -86,23 +91,23 @@ pub const Rational = struct { return .{ .num = num, .den = den }; } - /// Reduce by the GCD and force the sign onto the numerator. - fn normalize(self: *Rational, allocator: Allocator) Error!void { - if (self.num.eqlZero()) { - try self.den.set(1); - self.num.setSign(true); + /// Reduce by the GCD and force the sign onto the numerator, in place. + fn normalizeParts(allocator: Allocator, num: *Managed, den: *Managed) Error!void { + if (num.eqlZero()) { + try den.set(1); + num.setSign(true); return; } // Move the sign to the numerator. - if (!self.den.isPositive()) { - self.num.negate(); - self.den.negate(); + if (!den.isPositive()) { + num.negate(); + den.negate(); } var g = try Managed.init(allocator); defer g.deinit(); - try g.gcd(&self.num, &self.den); + try g.gcd(num, den); // Nothing to do when already coprime. if (g.toConst().orderAgainstScalar(1) == .eq) return; @@ -112,10 +117,10 @@ pub const Rational = struct { var r = try Managed.init(allocator); defer r.deinit(); - try q.divFloor(&r, &self.num, &g); - try self.num.copy(q.toConst()); - try q.divFloor(&r, &self.den, &g); - try self.den.copy(q.toConst()); + try q.divFloor(&r, num, &g); + try num.copy(q.toConst()); + try q.divFloor(&r, den, &g); + try den.copy(q.toConst()); } // -- Parsing -- @@ -201,7 +206,7 @@ pub const Rational = struct { } if (negative) num.negate(); - return initOwned(allocator, num, den); + return finish(allocator, &num, &den); } // -- Predicates -- @@ -285,7 +290,7 @@ pub const Rational = struct { errdefer den.deinit(); try den.mul(&a.den, &b.den); - return initOwned(allocator, num, den); + return finish(allocator, &num, &den); } pub fn sub(allocator: Allocator, a: Rational, b: Rational) Error!Rational { @@ -304,7 +309,7 @@ pub const Rational = struct { errdefer den.deinit(); try den.mul(&a.den, &b.den); - return initOwned(allocator, num, den); + return finish(allocator, &num, &den); } pub fn mul(allocator: Allocator, a: Rational, b: Rational) Error!Rational { @@ -316,7 +321,7 @@ pub const Rational = struct { errdefer den.deinit(); try den.mul(&a.den, &b.den); - return initOwned(allocator, num, den); + return finish(allocator, &num, &den); } pub fn div(allocator: Allocator, a: Rational, b: Rational) Error!Rational { @@ -330,7 +335,7 @@ pub const Rational = struct { errdefer den.deinit(); try den.mul(&a.den, &b.num); - return initOwned(allocator, num, den); + return finish(allocator, &num, &den); } pub fn negate(allocator: Allocator, a: Rational) Error!Rational { @@ -374,9 +379,9 @@ pub const Rational = struct { if (exponent < 0) { // Reciprocal: swap, then let normalize fix the sign. - return initOwned(allocator, den, num); + return finish(allocator, &den, &num); } - return initOwned(allocator, num, den); + return finish(allocator, &num, &den); } /// Exact square root, or null when the value is not a perfect square of a @@ -416,7 +421,7 @@ pub const Rational = struct { return null; } - return try initOwned(allocator, num_root, den_root); + return try finish(allocator, &num_root, &den_root); } /// Largest integer not greater than the value. @@ -431,8 +436,9 @@ pub const Rational = struct { // exactly floor for a positive denominator (an invariant here). try q.divFloor(&r, &a.num, &a.den); - const den = try Managed.initSet(allocator, 1); - return initOwned(allocator, q, den); + var den = try Managed.initSet(allocator, 1); + errdefer den.deinit(); + return finish(allocator, &q, &den); } /// Smallest integer not less than the value. @@ -491,8 +497,9 @@ pub const Rational = struct { defer factor.deinit(); try acc.mul(&acc, &factor); } - const den = try Managed.initSet(allocator, 1); - return initOwned(allocator, acc, den); + var den = try Managed.initSet(allocator, 1); + errdefer den.deinit(); + return finish(allocator, &acc, &den); } // -- Conversion -- @@ -1430,3 +1437,152 @@ test "factorial: 20 is exact where f64 is already lossy" { test "factorial: absurd inputs are rejected" { try testing.expectError(Error.ExponentTooLarge, Rational.factorial(alloc, 20_001)); } + +// -- Allocation-failure safety -- +// +// Every `errdefer` in this file exists to release a partially-constructed value +// when a LATER allocation fails. Merely executing those lines proves nothing; +// what matters is that no memory leaks when the failure happens. +// +// `std.testing.FailingAllocator` fails after exactly N allocations, so sweeping +// N across an operation's whole allocation sequence drives every intermediate +// failure point. `testing.allocator` sits underneath and reports a leak at the +// end of the test, which is what actually verifies the errdefer chain. + +/// Run `body` repeatedly, failing the 0th allocation, then the 1st, and so on, +/// until the operation completes without needing to fail. Any error other than +/// OutOfMemory is a real failure. +fn oomSweep(comptime body: fn (Allocator) anyerror!void) !void { + var fail_index: usize = 0; + while (fail_index < 512) : (fail_index += 1) { + var failing = std.testing.FailingAllocator.init(alloc, .{ .fail_index = fail_index }); + if (body(failing.allocator())) |_| { + // Completed without exhausting the budget: the sweep is done. + return; + } else |err| { + if (err != error.OutOfMemory) return err; + } + } + return error.OomSweepNeverCompleted; +} + +fn bodyParseDecimal(a: Allocator) anyerror!void { + var x = try Rational.parseDecimal(a, "-1.25e3"); + defer x.deinit(); + var y = try Rational.parseDecimal(a, "0.1"); + defer y.deinit(); +} + +fn bodyInitRatio(a: Allocator) anyerror!void { + var x = try Rational.initRatio(a, 6, -8); + defer x.deinit(); + var y = try x.clone(); + defer y.deinit(); +} + +fn bodyArithmetic(a: Allocator) anyerror!void { + var x = try Rational.parseDecimal(a, "1.5"); + defer x.deinit(); + var y = try Rational.parseDecimal(a, "2.25"); + defer y.deinit(); + + var sum = try Rational.add(a, x, y); + defer sum.deinit(); + var diff = try Rational.sub(a, x, y); + defer diff.deinit(); + var prod = try Rational.mul(a, x, y); + defer prod.deinit(); + var quot = try Rational.div(a, x, y); + defer quot.deinit(); + var neg = try Rational.negate(a, x); + defer neg.deinit(); + var magnitude = try Rational.abs(a, neg); + defer magnitude.deinit(); + _ = try Rational.order(a, x, y); +} + +fn bodyPowAndSqrt(a: Allocator) anyerror!void { + var base = try Rational.initRatio(a, 9, 16); + defer base.deinit(); + var p = try Rational.powInt(a, base, 3); + defer p.deinit(); + var q = try Rational.powInt(a, base, -2); + defer q.deinit(); + if (try Rational.sqrtExact(a, base)) |root| { + var r = root; + r.deinit(); + } + var two = try Rational.initInt(a, 2); + defer two.deinit(); + // A non-square: exercises the path that allocates roots, discovers they do + // not square back, and releases them before returning null. + try std.testing.expect((try Rational.sqrtExact(a, two)) == null); +} + +fn bodyRounding(a: Allocator) anyerror!void { + var x = try Rational.parseDecimal(a, "-3.7"); + defer x.deinit(); + var f = try Rational.floor(a, x); + defer f.deinit(); + var c = try Rational.ceil(a, x); + defer c.deinit(); + var r = try Rational.round(a, x); + defer r.deinit(); + + var three = try Rational.initInt(a, 3); + defer three.deinit(); + var m = try Rational.mod(a, x, three); + defer m.deinit(); +} + +fn bodyFactorial(a: Allocator) anyerror!void { + var f = try Rational.factorial(a, 12); + defer f.deinit(); +} + +fn bodyRendering(a: Allocator) anyerror!void { + var x = try Rational.initRatio(a, 1, 7); + defer x.deinit(); + + const frac = try x.toFractionString(a); + a.free(frac); + + const repeating = try x.toDecimalString(a, 12); + a.free(repeating.text); + + var terminating = try Rational.parseDecimal(a, "0.125"); + defer terminating.deinit(); + const exact = try terminating.toDecimalString(a, 12); + a.free(exact.text); + + _ = try x.isTerminating(a); + _ = x.toFloat(a); +} + +test "OOM safety: parseDecimal leaks nothing at any failure point" { + try oomSweep(bodyParseDecimal); +} + +test "OOM safety: initRatio and clone" { + try oomSweep(bodyInitRatio); +} + +test "OOM safety: the four arithmetic operations plus negate, abs and order" { + try oomSweep(bodyArithmetic); +} + +test "OOM safety: powInt and sqrtExact" { + try oomSweep(bodyPowAndSqrt); +} + +test "OOM safety: floor, ceil, round and mod" { + try oomSweep(bodyRounding); +} + +test "OOM safety: factorial" { + try oomSweep(bodyFactorial); +} + +test "OOM safety: decimal and fraction rendering" { + try oomSweep(bodyRendering); +}