Artifact c4f044ebc6abdf46d0d61e4ae870ca1aa3c76487d61e14ddd61f14dfc05f69b0:
- File
src/release_assert_test.zig
— part of check-in
[949e2b5542]
at
2026-07-30 21:47:09
on branch trunk
— release_assert: a guard against assert used as input validation
std.debug.assert is right for an internal invariant and wrong for a rule about data the function did not produce, and it is wrong in two different ways. Under the ReleaseSafe default it survives and PANICS: on the write path that is the trade we want, a crash over a durable bad frame, but on a peer-reachable path it is a remote denial of service. Under an explicitly chosen --release=fast it compiles to nothing and the input flows through unvalidated. One fix, a checked error, closes both.
Flags an assert only when all of: the fn is pub, returns an error union so a rejection was already available, the assert names one of its own parameters, and no earlier return in the body already rejects on that parameter. That last rule is what makes it signal -- delta.applyCopy returns MalformedDelta and then asserts to restate it, which is the CORRECT shape and must not be flagged.
The baseline is keyed on file, function and COUNT, never on line numbers: line numbers churn on every edit above them, while a count still catches a second bad assert added to a function that already has one. It fails in both directions, so a fixed finding must be removed from the list and the list can only shrink.
Existing findings are recorded, not fixed. Triage belongs in issue 19, where the ReleaseSafe default already reduces the write-path subset to crash-instead-of-corrupt and leaves the sync-side ones as the live exposure. (user: dmestas, size: 38737) [annotate] [blame] [check-ins using] [more...]
//! release_assert: a guard against `std.debug.assert` used as input validation. //! //! `std.debug.assert` is the right tool for an *internal invariant* — a fact the //! function itself established, held to as a crash-on-violation. It is the wrong //! tool for a rule about data the function did not produce, and it is wrong in //! two different ways depending on how the binary was built. Neither of them is //! "the input gets rejected", which is what the code reads as if it does. //! //! Under ReleaseSafe — which build.zig now makes the default for `--release`, //! pinned by `src/ci_test.zig`'s "the shipped release mode keeps assertions //! alive" — the assert survives and **panics**. On the write path that is a good //! trade and deliberately chosen: a crash beats a durable bad frame in an //! append-only store. On a *peer-reachable* path it is not a trade at all, it is //! a remote denial of service — anyone who can hand this process a malformed //! card can halt it. A parser reached from the sync wire must reject its input, //! not assert about it, however safe the build mode is. //! //! Under ReleaseFast or ReleaseSmall the assert compiles to nothing and the //! malformed input flows straight through into the work the assert was meant to //! stop. The ReleaseSafe pin makes this the less likely build, not an impossible //! one: it sets a default, not a ceiling, and `--release=fast` still selects it //! deliberately. A guard that only holds while nobody passes a flag is not a //! guard. //! //! So the same finding names a DoS in the default build and a missing check in a //! fast one, and the same one-line fix — `return error.X` — closes both. //! //! Three defects of exactly this shape were found and fixed recently: //! //! * `putShun` asserted a bound on caller-supplied bytes and, with the assert //! compiled out, wrote a durable frame that could never be opened again. //! * `ShunSet.contains` asserted a card's shape and crashed on a peer's //! malformed card instead of rejecting it. //! * `gatherCandidates` asserted a caller-supplied bound and, without it, left //! a scan unbounded. //! //! One of each: `ShunSet.contains` is the panic-on-a-peer's-input shape that //! ReleaseSafe does nothing to help, and the other two are the compiled-out //! shape. All three were `pub fn`s that already returned an error union — the //! correct fix in each case was `return error.X` rather than `assert`. This //! guard exists so the fourth one is caught by `zig build test` instead of by a //! user or a peer. //! //! What it flags — all of these must hold at once: //! //! 1. the enclosing function is `pub fn` (it is reachable from outside), //! 2. its return type is an error union (`!` or a named `…Error…` type), so //! rejecting the input costs nothing structurally, //! 3. the assert expression names one of that function's own parameters //! (`self`, `gpa`, `io`, `allocator`, `arena` are conventional plumbing and //! do not count), //! 4. that parameter is *untrusted*: a slice type, or a name from the //! `untrusted_names` list below, and //! 5. the assert is NOT already dominated by a `return` on that parameter. //! //! Rules 3 and 5 are what make this signal rather than noise, and `container.zig` //! is the worked example of each — it holds exactly two asserts and neither is a //! finding, for two different correct reasons: //! //! * `encode`'s assert names the parameter `bytes`, but the function opens with //! `if (bytes.len > …) return error.InputTooLarge`. The rejection already //! happened; the assert restates an established fact. Rule 5 clears it. //! * `decode`'s assert names only locals (`out`, `total`, `declared`). It is a //! postcondition about the function's own output, not a rule about input. //! Rule 3 clears it. //! //! `delta.applyCopy` is the same shape as the first (and is private besides, so //! rule 1 excludes it too). Because those zeros are load-bearing — a parser that //! silently stopped seeing asserts would also produce them — the test //! "container.zig's two asserts are attributed and correctly cleared" below pins //! the attribution directly rather than trusting the absence. //! //! The baseline is keyed on **(file, function name)** with a COUNT, deliberately //! not on line numbers: line numbers churn on every edit above them and would //! turn this into a maintenance tax. The count is what stops a function that //! already asserts badly — the likeliest place for the next bad assert — from //! absorbing a new one for free. A count that rises fails; a count that falls //! also fails, so the list can only shrink. //! //! The parse is driven by the Zig tokenizer, so an `assert` written inside a //! comment or a string literal is not read as a call, and a `{` inside one //! cannot skew a function's body extent. It is still a heuristic, not semantic //! analysis: it does not resolve aliases (`const assert = std.debug.assert`) or //! types. That is a knowing trade — the shape it does catch is the shape that //! produced all three defects. const std = @import("std"); /// The repository root, wired in build.zig's `build_unit_test_module`. A test /// binary's working directory is not the checkout, so the src tree is opened by /// this absolute path. Same handle src/tidy.zig and src/packaging_test.zig use. const repo_options = @import("repo_options"); /// Parameter names that are plumbing, not data: an assert on one of these is /// about the call's own wiring, not about untrusted content. const plumbing_names = [_][]const u8{ "self", "gpa", "io", "allocator", "arena" }; /// Substrings that mark a parameter as carrying data the function did not /// produce. A slice-typed parameter is untrusted regardless of its name; these /// catch the non-slice carriers (a length, an index, a scalar id read off the /// wire) whose names say where the value came from. const untrusted_names = [_][]const u8{ "hash", "name", "card", "payload", "bytes", "buf", "slice", "priority", "deltasrc", "token", "reason", "digest", "wire", "input", }; /// Files not scanned. Test and simulation sources assert about fixtures they /// themselves built — the parameter is trusted by construction — and the three /// harness roots (`e2e`, `smoke`, `dst`) are drivers, not library surface. const excluded_prefixes = [_][]const u8{"sim/"}; const excluded_files = [_][]const u8{ "e2e.zig", "smoke.zig", "dst.zig" }; /// Vacuity floor: the walk must find at least this many scannable `.zig` files /// or the tree was not actually scanned and every check below would pass by /// finding nothing. ~45 files survive the exclusions today; 30 is a floor with /// headroom, not a running count to keep in sync. const min_expected_files = 30; /// Read cap per source file. The largest src file is well under 1 MiB; 8 MiB is /// a tripwire against a runaway read, not an expected size. const max_source_bytes = 8 << 20; /// One baselined function and how many undominated asserts on untrusted input it /// held when the guard landed. const Baseline = struct { file: []const u8, function: []const u8, count: usize }; /// The pre-existing population, as measured by this guard's own parse. Each /// entry is a `pub fn` returning an error union that asserts on untrusted input /// without a dominating `return` — an input path that panics where it should /// reject (and, in a `--release=fast` build, does not check at all). /// /// These are recorded rather than fixed in one sweep because each fix is an API /// decision (which error, and what every caller does with it). The 29 entries in /// `sync.zig` are the ones to fix first: they are wire-card parsers, so under the /// ReleaseSafe default they are live remote-DoS surface rather than a latent /// fast-build hole. /// /// Counts may only fall. Raising one is not a fix — see issue #19. const baseline = [_]Baseline{ .{ .file = "annotate.zig", .function = "attribute", .count = 1 }, .{ .file = "artifact.zig", .function = "resolvePrefix", .count = 2 }, .{ .file = "checkout.zig", .function = "read", .count = 1 }, .{ .file = "cli.zig", .function = "composeAmendTags", .count = 1 }, .{ .file = "cli.zig", .function = "composeTagAddCards", .count = 1 }, .{ .file = "delta.zig", .function = "create", .count = 2 }, .{ .file = "logstore.zig", .function = "intern", .count = 1 }, .{ .file = "logstore.zig", .function = "pickBase", .count = 2 }, .{ .file = "logstore.zig", .function = "put", .count = 1 }, .{ .file = "logstore.zig", .function = "putArtifact", .count = 2 }, .{ .file = "logstore.zig", .function = "putArtifactPacked", .count = 2 }, .{ .file = "logstore.zig", .function = "putContainer", .count = 1 }, .{ .file = "logstore.zig", .function = "putDelta", .count = 1 }, .{ .file = "logstore.zig", .function = "putDeltaContainer", .count = 1 }, .{ .file = "logstore.zig", .function = "putZlib", .count = 1 }, .{ .file = "logstore.zig", .function = "resolveHexPrefix", .count = 1 }, .{ .file = "main.zig", .function = "mintControlArtifact", .count = 1 }, .{ .file = "repoimage.zig", .function = "serialize", .count = 1 }, .{ .file = "repostate.zig", .function = "bisectSpan", .count = 2 }, .{ .file = "repostate.zig", .function = "currentTrunk", .count = 1 }, .{ .file = "repostate.zig", .function = "ingestControlTags", .count = 1 }, .{ .file = "repostate.zig", .function = "openLeafOf", .count = 2 }, .{ .file = "repostate.zig", .function = "openLeavesOf", .count = 2 }, .{ .file = "repostate.zig", .function = "putWire", .count = 1 }, .{ .file = "repostate.zig", .function = "putWireDelta", .count = 2 }, .{ .file = "repostate.zig", .function = "resolve", .count = 1 }, .{ .file = "repostate.zig", .function = "resolveName", .count = 1 }, .{ .file = "repoview.zig", .function = "buildFromImage", .count = 1 }, .{ .file = "repoview.zig", .function = "buildFromRecordBody", .count = 1 }, .{ .file = "sync.zig", .function = "apply", .count = 2 }, .{ .file = "sync.zig", .function = "consumeConfigPayload", .count = 1 }, .{ .file = "sync.zig", .function = "consumeContentPayload", .count = 1 }, .{ .file = "sync.zig", .function = "consumeReply", .count = 1 }, .{ .file = "sync.zig", .function = "consumeUvfilePayload", .count = 1 }, .{ .file = "sync.zig", .function = "fossilUnquote", .count = 1 }, .{ .file = "sync.zig", .function = "parseCfileCard", .count = 3 }, .{ .file = "sync.zig", .function = "parseConfigCard", .count = 2 }, .{ .file = "sync.zig", .function = "parseFileCard", .count = 3 }, .{ .file = "sync.zig", .function = "parseGimmeCard", .count = 2 }, .{ .file = "sync.zig", .function = "parseIgotCard", .count = 2 }, .{ .file = "sync.zig", .function = "parseUvfileCard", .count = 2 }, .{ .file = "sync.zig", .function = "parseUvgimmeCard", .count = 2 }, .{ .file = "sync.zig", .function = "parseUvigotCard", .count = 2 }, .{ .file = "sync.zig", .function = "readLine", .count = 2 }, .{ .file = "sync.zig", .function = "recordPeerHas", .count = 1 }, .{ .file = "sync.zig", .function = "recordPeerLacks", .count = 1 }, .{ .file = "sync.zig", .function = "writeCiLock", .count = 3 }, .{ .file = "sync.zig", .function = "writeCiLockFail", .count = 2 }, .{ .file = "sync.zig", .function = "writeCiUnlock", .count = 2 }, .{ .file = "sync.zig", .function = "writeClientVersion", .count = 1 }, .{ .file = "sync.zig", .function = "writeCloneCard", .count = 1 }, .{ .file = "sync.zig", .function = "writeConfigCard", .count = 2 }, .{ .file = "sync.zig", .function = "writeFlagPragma", .count = 2 }, .{ .file = "sync.zig", .function = "writeLogin", .count = 3 }, .{ .file = "sync.zig", .function = "writePushPullCard", .count = 3 }, .{ .file = "sync.zig", .function = "writeReqconfig", .count = 3 }, .{ .file = "sync.zig", .function = "writeScalarConfig", .count = 1 }, .{ .file = "sync.zig", .function = "writeServerVersion", .count = 1 }, .{ .file = "undo.zig", .function = "record", .count = 1 }, }; test "no new std.debug.assert guards a release-build rule on untrusted input" { const io = std.testing.io; var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); const src_abs = try std.fmt.allocPrint(arena, "{s}/src", .{repo_options.root}); var src_dir = std.Io.Dir.cwd().openDir(io, src_abs, .{ .iterate = true }) catch |err| switch (err) { // The only legitimate skip: the wired root is not a directory here, so // the machine cannot answer the question. Anything else is a real fault. error.FileNotFound, error.NotDir => return error.SkipZigTest, else => { std.debug.print("\nrelease_assert: cannot open {s}: {s}\n", .{ src_abs, @errorName(err) }); return error.ReleaseAssertWalkFailed; }, }; defer src_dir.close(io); var files: std.ArrayList([]const u8) = .empty; { var walker = try src_dir.walk(arena); defer walker.deinit(); while (try walker.next(io)) |entry| { if (entry.kind != .file) continue; if (!std.mem.endsWith(u8, entry.path, ".zig")) continue; if (isExcluded(entry.path)) continue; try files.append(arena, try arena.dupe(u8, entry.path)); } } std.mem.sort([]const u8, files.items, {}, lessThanPath); if (files.items.len < min_expected_files) { std.debug.print("\nrelease_assert: walked {s} and found only {d} scannable .zig files; " ++ "expected at least {d}. The walk is not seeing the source tree — this guard " ++ "would pass vacuously.\n", .{ src_abs, files.items.len, min_expected_files }); return error.ReleaseAssertWalkFailed; } var scan: Scan = .empty; for (files.items) |file| { const source = try src_dir.readFileAllocOptions(io, file, arena, .limited(max_source_bytes), .of(u8), 0); try scanFile(arena, file, source, &scan); } var failed = false; // Forward direction: more asserts in a function than the baseline records. // A brand-new offender in a clean function and a second offender bolted onto // an already-listed one are the same failure and read the same way. for (scan.entries.values()) |entry| { const recorded = baselineCount(entry.file, entry.function); if (entry.sites.items.len <= recorded) continue; failed = true; std.debug.print( \\ \\release_assert: src/{s}: `{s}` has {d} assert(s) on untrusted input; the baseline records {d}. \\ , .{ entry.file, entry.function, entry.sites.items.len, recorded }); for (entry.sites.items) |site| { std.debug.print(" line {d} (`{s}`): {s}\n", .{ site.line, site.param, site.source_line }); } std.debug.print( \\ An assert is not a rejection. Under the default ReleaseSafe build these \\ panic, so if that data can reach `{s}` from a peer or a repository file, \\ that is a remote denial of service. Under an explicitly chosen \\ `--release=fast` build they compile to nothing and the data flows through \\ unvalidated instead. One fix closes both: `{s}` already returns an error \\ union — return an error. \\ if (!condition) return error.SomethingSpecific; \\ If the fact really is an invariant this function established, establish it \\ first (a preceding `return` on that parameter) and the assert then restates \\ it, which this guard accepts. \\ Every site in the function is listed above, not just the new one — the \\ baseline records a count, not positions. See issue #19. \\ , .{ entry.function, entry.function }); } // Reverse direction: fewer than recorded means someone fixed one. Say so as // progress, and require the number to come down so the list can only shrink. var key_buffer: [key_max]u8 = undefined; for (baseline) |entry| { const actual = if (scan.entries.get(entryKey(&key_buffer, entry.file, entry.function))) |found| found.sites.items.len else 0; if (actual >= entry.count) continue; failed = true; if (actual == 0) { std.debug.print( \\ \\release_assert: the baseline lists src/{s} `{s}` with {d} assert(s), but it now \\ has none. That is one fewer input path that panics instead of rejecting — \\ remove the entry from the baseline in src/release_assert_test.zig so the \\ list keeps shrinking. \\ (If instead the function was renamed or moved, update the entry to match.) \\ , .{ entry.file, entry.function, entry.count }); } else { std.debug.print( \\ \\release_assert: the baseline lists src/{s} `{s}` with {d} assert(s), but it now \\ has {d}. That is progress — lower the count in the baseline in \\ src/release_assert_test.zig so it keeps shrinking. \\ , .{ entry.file, entry.function, entry.count, actual }); } } if (failed) return error.ReleaseAssertOnUntrustedInput; } /// A source fixture with a known answer, so the parser's attribution is pinned by /// something that cannot drift underneath it. Every shape here previously had to /// be reasoned about by hand; now it is executable. const fixture = \\const std = @import("std"); \\ \\pub fn flagged(card: []const u8) !void { \\ std.debug.assert(card.len > 4); \\} \\ \\pub fn cleared(card: []const u8) !void { \\ if (card.len < 4) return error.Short; \\ std.debug.assert(card.len > 4); \\} \\ \\pub fn bracesInText(payload: []const u8) !void { \\ // an unbalanced brace in a comment: { \\ const s = "an unbalanced brace in a string: {"; \\ _ = s; \\ switch (payload.len) { \\ 0 => {}, \\ else => {}, \\ } \\ errdefer { \\ _ = payload; \\ } \\ std.debug.assert(payload.len > 1); \\} \\ \\pub fn outer(bytes: []const u8) !void { \\ const Inner = struct { \\ pub fn nested(digest: []const u8) !void { \\ std.debug.assert(digest.len == 32); \\ } \\ }; \\ try Inner.nested(bytes); \\ std.debug.assert(bytes.len != 0); \\} \\ \\pub fn notAnErrorUnion(buf: []const u8) void { \\ std.debug.assert(buf.len > 0); \\} \\ \\fn notPublic(buf: []const u8) !void { \\ std.debug.assert(buf.len > 0); \\} \\ \\pub fn onlyLocals(wire: []const u8) !void { \\ const n = wire.len; \\ const doubled = n * 2; \\ std.debug.assert(doubled >= 0); \\} \\ \\pub fn twoAsserts(hash: []const u8) !void { \\ std.debug.assert(hash.len == 40); \\ std.debug.assert(hash[0] != 0); \\} ; test "the scanner attributes asserts to the right function" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); var scan: Scan = .empty; try scanFile(arena, "fixture.zig", fixture, &scan); // Vacuity: every `std.debug.assert` in the fixture must have been seen and // attributed, or the filters below would be judging an empty set. try std.testing.expectEqual(@as(usize, 10), scan.asserts_seen); // Attribution, independent of whether the assert is a finding. `nested` is // the inner `pub fn` of a struct declared inside `outer`, so it exercises the // innermost-span rule; `notPublic` has no enclosing *candidate* and resolves // to "". const expected_owners = [_][]const u8{ "flagged", "cleared", "bracesInText", "nested", "outer", "", "", "onlyLocals", "twoAsserts", "twoAsserts", }; try std.testing.expectEqual(expected_owners.len, scan.attributions.items.len); for (expected_owners, scan.attributions.items) |want, got| { try std.testing.expectEqualStrings(want, got.function); // Self-check the line number rather than hand-counting it: the line the // scanner reported must itself hold the assert. try std.testing.expect(std.mem.indexOf(u8, nthLine(fixture, got.line), "std.debug.assert") != null); } // Findings, with counts. `cleared` is dominated (rule 5), `notAnErrorUnion` // and `notPublic` fail rules 2 and 1, and `onlyLocals` names no parameter // (rule 3) even though `wire` is in scope and untrusted. const expected = [_]struct { function: []const u8, count: usize }{ .{ .function = "flagged", .count = 1 }, .{ .function = "bracesInText", .count = 1 }, .{ .function = "nested", .count = 1 }, .{ .function = "outer", .count = 1 }, .{ .function = "twoAsserts", .count = 2 }, }; try std.testing.expectEqual(expected.len, scan.entries.count()); var key_buffer: [key_max]u8 = undefined; for (expected) |want| { const got = scan.entries.get(entryKey(&key_buffer, "fixture.zig", want.function)) orelse { std.debug.print("\nrelease_assert fixture: expected a finding in `{s}`, got none\n", .{want.function}); return error.FixtureMismatch; }; try std.testing.expectEqual(want.count, got.sites.items.len); } } test "container.zig's two asserts are attributed and correctly cleared" { const io = std.testing.io; var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); const path = try std.fmt.allocPrint(arena, "{s}/src/container.zig", .{repo_options.root}); const source = std.Io.Dir.cwd().readFileAllocOptions(io, path, arena, .limited(max_source_bytes), .of(u8), 0) catch |err| switch (err) { error.FileNotFound => return error.SkipZigTest, else => return err, }; var scan: Scan = .empty; try scanFile(arena, "container.zig", source, &scan); // The file holds exactly two asserts, one per public function. Pinning the // attribution is the point: the zero findings below are only meaningful if // the scanner actually resolved both, rather than losing them to a body-extent // bug and reporting nothing. try std.testing.expectEqual(@as(usize, 2), scan.asserts_seen); try std.testing.expectEqual(@as(usize, 2), scan.attributions.items.len); try std.testing.expectEqualStrings("encode", scan.attributions.items[0].function); try std.testing.expectEqualStrings("decode", scan.attributions.items[1].function); // Neither is a finding: `encode`'s is dominated by its opening // `return error.InputTooLarge` on `bytes` (rule 5), and `decode`'s names only // locals (rule 3). See the module doc. try std.testing.expectEqual(@as(usize, 0), scan.entries.count()); } /// The `line`-th line of `source`, 1-indexed. fn nthLine(source: []const u8, line: usize) []const u8 { var it = std.mem.splitScalar(u8, source, '\n'); var n: usize = 1; while (it.next()) |text| : (n += 1) { if (n == line) return text; } return ""; } /// One assert that is a finding. const Site = struct { line: usize, param: []const u8, source_line: []const u8, }; /// All findings for one (file, function) pair. const Entry = struct { file: []const u8, function: []const u8, sites: std.ArrayList(Site), }; /// Every assert the scanner resolved, finding or not. Kept so the fixture tests /// can pin attribution independently of the filters, and so a parser that stopped /// seeing asserts cannot masquerade as a clean tree. const Attribution = struct { line: usize, function: []const u8 }; const Scan = struct { entries: std.StringArrayHashMapUnmanaged(Entry), attributions: std.ArrayList(Attribution), asserts_seen: usize, const empty: Scan = .{ .entries = .empty, .attributions = .empty, .asserts_seen = 0 }; }; /// The map key for one (file, function) pair, written into a caller-owned /// buffer. The map compares keys by bytes, so a lookup key only has to outlive /// the call; `scanFile` dupes into the arena on insert. /// /// A path plus an identifier cannot approach 512 bytes in this tree, and the /// alternative to a fixed buffer — allocating a key for every lookup — buys /// nothing. Truncation would silently merge two functions' counts, so it fails /// loudly instead. const key_max = 512; fn entryKey(buffer: *[key_max]u8, file: []const u8, function: []const u8) []const u8 { return std.fmt.bufPrint(buffer, "{s}\x00{s}", .{ file, function }) catch @panic("release_assert: (file, function) key exceeds 512 bytes"); } fn lessThanPath(_: void, a: []const u8, b: []const u8) bool { return std.mem.order(u8, a, b) == .lt; } fn isExcluded(path: []const u8) bool { if (std.mem.endsWith(u8, path, "_test.zig")) return true; for (excluded_prefixes) |prefix| { if (std.mem.startsWith(u8, path, prefix)) return true; } for (excluded_files) |file| { if (std.mem.eql(u8, path, file)) return true; } return false; } fn baselineCount(file: []const u8, function: []const u8) usize { for (baseline) |entry| { if (std.mem.eql(u8, entry.file, file) and std.mem.eql(u8, entry.function, function)) return entry.count; } return 0; } const Param = struct { name: []const u8, untrusted: bool }; /// One `pub fn` with an error-union return type, and the byte span of its body. const Function = struct { name: []const u8, params: []const Param, body_start: usize, body_end: usize, }; /// Tokenize `source`, collect every candidate `pub fn`, then attribute each /// `std.debug.assert(...)` call to the innermost one containing it and apply the /// untrusted-parameter and dominance rules. fn scanFile( arena: std.mem.Allocator, file: []const u8, source: [:0]const u8, scan: *Scan, ) !void { var tokens: std.ArrayList(std.zig.Token) = .empty; { var tokenizer = std.zig.Tokenizer.init(source); while (true) { const token = tokenizer.next(); try tokens.append(arena, token); if (token.tag == .eof) break; } } const toks = tokens.items; var functions: std.ArrayList(Function) = .empty; for (toks, 0..) |token, i| { if (token.tag != .keyword_pub) continue; if (i + 1 >= toks.len or toks[i + 1].tag != .keyword_fn) continue; const function = try parseFunction(arena, source, toks, i + 1) orelse continue; try functions.append(arena, function); } var i: usize = 0; while (i + 5 < toks.len) : (i += 1) { if (!isDottedCall(source, toks, i, "std", "debug", "assert")) continue; const arg_end = matchingParen(toks, i + 5) orelse continue; const assert_start = toks[i].loc.start; scan.asserts_seen += 1; const enclosing = innermost(functions.items, assert_start); try scan.attributions.append(arena, .{ .line = lineNumber(source, assert_start), .function = if (enclosing) |function| function.name else "", }); const owner = enclosing orelse continue; for (owner.params) |param| { if (!param.untrusted) continue; if (!spanMentionsIdentifier(source, toks, i + 6, arg_end, param.name)) continue; if (dominated(source, owner.body_start, assert_start, param.name)) continue; const site: Site = .{ .line = lineNumber(source, assert_start), .param = param.name, .source_line = std.mem.trim(u8, lineAt(source, assert_start), " \t"), }; var key_buffer: [key_max]u8 = undefined; const gop = try scan.entries.getOrPut(arena, entryKey(&key_buffer, file, owner.name)); if (!gop.found_existing) { gop.key_ptr.* = try arena.dupe(u8, gop.key_ptr.*); gop.value_ptr.* = .{ .file = file, .function = owner.name, .sites = .empty }; } try gop.value_ptr.sites.append(arena, site); // One finding per assert, not per matching parameter: an assert that // names two untrusted parameters is still one thing to fix. break; } } } /// True when tokens `[i..i+5]` spell `a.b.c` followed by `(`. fn isDottedCall( source: []const u8, toks: []const std.zig.Token, i: usize, a: []const u8, b: []const u8, c: []const u8, ) bool { if (toks[i].tag != .identifier) return false; if (!std.mem.eql(u8, tokenText(source, toks[i]), a)) return false; if (toks[i + 1].tag != .period) return false; if (toks[i + 2].tag != .identifier or !std.mem.eql(u8, tokenText(source, toks[i + 2]), b)) return false; if (toks[i + 3].tag != .period) return false; if (toks[i + 4].tag != .identifier or !std.mem.eql(u8, tokenText(source, toks[i + 4]), c)) return false; return toks[i + 5].tag == .l_paren; } fn tokenText(source: []const u8, token: std.zig.Token) []const u8 { return source[token.loc.start..token.loc.end]; } /// Index of the `)` matching the `(` at `open`, or null if unbalanced. fn matchingParen(toks: []const std.zig.Token, open: usize) ?usize { var depth: usize = 0; var i = open; while (i < toks.len) : (i += 1) { switch (toks[i].tag) { .l_paren => depth += 1, .r_paren => { depth -= 1; if (depth == 0) return i; }, .eof => return null, else => {}, } } return null; } /// Index of the `}` matching the `{` at `open`, or null if unbalanced. fn matchingBrace(toks: []const std.zig.Token, open: usize) ?usize { var depth: usize = 0; var i = open; while (i < toks.len) : (i += 1) { switch (toks[i].tag) { .l_brace => depth += 1, .r_brace => { depth -= 1; if (depth == 0) return i; }, .eof => return null, else => {}, } } return null; } /// Parse the `pub fn` whose `fn` keyword is at `fn_index`. Returns null when it /// is not a candidate: no body (a prototype), or a return type that is not an /// error union — a function that cannot report a rejection is out of scope here. fn parseFunction( arena: std.mem.Allocator, source: []const u8, toks: []const std.zig.Token, fn_index: usize, ) !?Function { var i = fn_index + 1; if (i >= toks.len or toks[i].tag != .identifier) return null; const name = tokenText(source, toks[i]); i += 1; if (i >= toks.len or toks[i].tag != .l_paren) return null; const params_open = i; const params_close = matchingParen(toks, params_open) orelse return null; // The return type runs from just past `)` to the `{` that opens the body. // A `struct`/`union`/`enum`/`opaque`/`error` brace group inside the return // type is skipped so it is not mistaken for the body. var body_open: ?usize = null; var j = params_close + 1; while (j < toks.len) : (j += 1) { switch (toks[j].tag) { .semicolon, .eof => return null, // prototype, or a malformed parse .l_brace => { const introducer = if (j == 0) std.zig.Token.Tag.invalid else toks[j - 1].tag; const is_container = switch (introducer) { .keyword_struct, .keyword_union, .keyword_enum, .keyword_opaque, .keyword_error => true, else => false, }; if (is_container) { j = matchingBrace(toks, j) orelse return null; continue; } body_open = j; break; }, else => {}, } } const open = body_open orelse return null; const close = matchingBrace(toks, open) orelse return null; if (!returnsError(source, toks, params_close + 1, open)) return null; var params: std.ArrayList(Param) = .empty; try parseParams(arena, source, toks, params_open, params_close, ¶ms); if (params.items.len == 0) return null; return .{ .name = name, .params = params.items, .body_start = toks[open].loc.end, .body_end = toks[close].loc.start, }; } /// An error union return type: an explicit `!`, or a named type whose spelling /// carries `Error` (the `Foo.Error` / `SyncError` convention this repo uses). fn returnsError(source: []const u8, toks: []const std.zig.Token, from: usize, to: usize) bool { var i = from; while (i < to) : (i += 1) { if (toks[i].tag == .bang) return true; if (toks[i].tag == .identifier and std.mem.indexOf(u8, tokenText(source, toks[i]), "Error") != null) return true; } return false; } /// Split the parameter list at top-level commas and record each parameter's name /// and whether it is untrusted. Parameters without a `name: type` shape /// (`anytype` alone, `...`) contribute nothing and are skipped. fn parseParams( arena: std.mem.Allocator, source: []const u8, toks: []const std.zig.Token, open: usize, close: usize, out: *std.ArrayList(Param), ) !void { var start = open + 1; var i = start; var depth: usize = 0; while (i < close) : (i += 1) { switch (toks[i].tag) { .l_paren, .l_bracket, .l_brace => depth += 1, .r_paren, .r_bracket, .r_brace => depth -|= 1, .comma => if (depth == 0) { try pushParam(arena, source, toks, start, i, out); start = i + 1; }, else => {}, } } if (start < close) try pushParam(arena, source, toks, start, close, out); } fn pushParam( arena: std.mem.Allocator, source: []const u8, toks: []const std.zig.Token, from: usize, to: usize, out: *std.ArrayList(Param), ) !void { var i = from; while (i < to and (toks[i].tag == .keyword_comptime or toks[i].tag == .keyword_noalias)) i += 1; if (i >= to or toks[i].tag != .identifier) return; const name = tokenText(source, toks[i]); if (i + 1 >= to or toks[i + 1].tag != .colon) return; if (i + 2 >= to) return; // `name:` with no type — malformed, nothing to judge for (plumbing_names) |plumbing| { if (std.mem.eql(u8, name, plumbing)) return; } const type_text = std.mem.trim(u8, source[toks[i + 2].loc.start..toks[to - 1].loc.end], " \t\n"); var untrusted = std.mem.startsWith(u8, type_text, "[]"); if (!untrusted) { for (untrusted_names) |needle| { if (std.mem.indexOf(u8, name, needle) != null) { untrusted = true; break; } } } try out.append(arena, .{ .name = name, .untrusted = untrusted }); } /// The function with the smallest body span containing `offset`, so an assert in /// a nested `pub fn` is attributed to the nested one. fn innermost(functions: []const Function, offset: usize) ?Function { var best: ?Function = null; for (functions) |function| { if (offset < function.body_start or offset >= function.body_end) continue; const current = best orelse { best = function; continue; }; if (function.body_end - function.body_start < current.body_end - current.body_start) best = function; } return best; } /// True when some identifier token in `[from..to)` is exactly `name`. fn spanMentionsIdentifier( source: []const u8, toks: []const std.zig.Token, from: usize, to: usize, name: []const u8, ) bool { var i = from; while (i < to) : (i += 1) { if (toks[i].tag != .identifier) continue; if (std.mem.eql(u8, tokenText(source, toks[i]), name)) return true; } return false; } /// Rule 5. True when an earlier line of the same body both mentions `param` and /// contains `return` — the function has already had its chance to reject the /// input, so a following assert restates an established fact rather than /// standing in for validation. /// /// This is deliberately line-shaped rather than flow-sensitive: it is the /// pattern a reviewer reads off the page, and over-clearing here costs a missed /// finding while under-clearing would cost a false alarm on correct code. fn dominated(source: []const u8, body_start: usize, assert_start: usize, param: []const u8) bool { if (assert_start <= body_start) return false; const before = source[body_start..assert_start]; // Drop the partial final line: the assert's own line is not "earlier". const cut = std.mem.lastIndexOfScalar(u8, before, '\n') orelse return false; var it = std.mem.splitScalar(u8, before[0..cut], '\n'); while (it.next()) |line| { if (std.mem.indexOf(u8, line, "return") == null) continue; if (mentionsIdentifier(line, param)) return true; } return false; } /// Substring search for `name` bounded by non-identifier characters, so `hash` /// does not match inside `hash_index`. fn mentionsIdentifier(haystack: []const u8, name: []const u8) bool { var from: usize = 0; while (std.mem.indexOfPos(u8, haystack, from, name)) |at| { const before_ok = at == 0 or !isIdentChar(haystack[at - 1]); const after = at + name.len; const after_ok = after >= haystack.len or !isIdentChar(haystack[after]); if (before_ok and after_ok) return true; from = at + 1; } return false; } fn isIdentChar(c: u8) bool { return std.ascii.isAlphanumeric(c) or c == '_'; } fn lineNumber(source: []const u8, offset: usize) usize { return std.mem.count(u8, source[0..offset], "\n") + 1; } fn lineAt(source: []const u8, offset: usize) []const u8 { const start = if (std.mem.lastIndexOfScalar(u8, source[0..offset], '\n')) |at| at + 1 else 0; const rest = source[start..]; const end = std.mem.indexOfScalar(u8, rest, '\n') orelse rest.len; return rest[0..end]; }