From dd375aab9aa4b3c1ba8ba9bc785b83435a1429e9 Mon Sep 17 00:00:00 2001 From: Big Boss Date: Fri, 21 Aug 2026 12:01:12 -0500 Subject: [PATCH] fix(lint): clear the 14 findings ziglint reported on its first real run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `zig build lint` had never executed anywhere — it shells out to a binary nothing installed — so the findings had been accumulating unseen. With the tool now in the flake, it reports 14. 11 are Z030: deinit frees the struct's memory but leaves the struct readable, so a later use returns plausible bytes instead of failing. `self.* = undefined` poisons it, which in ReleaseSafe turns use-after-deinit into a loud crash. The GraphqlClient Response case is the one that matters: every parsed field is a slice into `parsed`'s arena, so any read after deinit was already use-after-free. bulk.Targets was resetting `items`/`storage` by hand at the end of deinit. It is only ever reached through `defer` at scope exit, so nothing could observe those resets — poisoning replaces them rather than adding to them. The rest: one anonymous struct binding `@This()` inline (now `const Self = @This()`), and `return Error.RequestTimedOut` -> `return error.RequestTimedOut`. ziglint flagged two of the three occurrences; the third is changed too, because leaving one behind would be the only reason a reader would think the two forms differ. They do not — Zig error sets are global, so both name the same error value. `pub const Error` stays as the module's declared error contract. CI gains a `lint` job running through the flake dev shell. A finding the harness should have caught earns a floor, not just a patch — without it this drifts straight back, which is how it got to 14. Verified: `zig build test` passes, `zig build lint` reports 0 findings, `zig fmt --check` clean. --- .github/workflows/ci.yml | 18 ++++++++++++++---- CHANGELOG.md | 4 ++++ src/commands/bulk.zig | 3 +-- src/config.zig | 1 + src/graphql_client.zig | 11 ++++++++--- src/tests/main.zig | 7 ++++++- src/tests/mock_graphql.zig | 4 +++- src/tests/online.zig | 1 + 8 files changed, 38 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cf53383..143a0cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,9 +1,5 @@ # CI: the offline verifier every push has to clear. # -# Deliberately not running ziglint — it is a `zig build lint` step that shells -# out to a binary this workflow would have to build from source on every run, -# and it is advisory. `zig fmt --check` and the unit suite are the floors. -# # The online suite (`zig build online`) is not here on purpose: it needs a real # LINEAR_API_KEY and mutates a live workspace. name: ci @@ -37,6 +33,20 @@ jobs: - name: Version manifests agree run: ./scripts/check-versions.sh + lint: + # ziglint runs through the flake dev shell, which is the only place the + # binary exists — `zig build lint` shells out to it and dies with + # FileNotFound otherwise. The tree is at zero findings; this is the floor + # that keeps it there. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - uses: DeterminateSystems/nix-installer-action@main + - uses: DeterminateSystems/magic-nix-cache-action@main + - run: nix develop -c zig build lint + cross-build: # The npm dist targets are cross-compiled and never exercised by `zig build # test`, so a target-specific break (libc, tcsetattr, file modes) only shows diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cf2558..3b0e1be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ Notable changes per release. Versions before 0.3.0 are recorded in the attestations automatically. - `flake.nix` provides a dev shell pinned to Zig 0.16.0 with `ziglint` and `jq`, so `zig build lint` works instead of failing with `FileNotFound`. +- The 14 findings that surfaced the first time ziglint actually ran are fixed + (11 `deinit` bodies now poison the struct with `self.* = undefined`, plus one + `@This()` binding and two error-literal returns), and CI runs `zig build + lint` through the flake so the tree stays at zero. ## 0.3.0 diff --git a/src/commands/bulk.zig b/src/commands/bulk.zig index ec3c48f..bd7e02d 100644 --- a/src/commands/bulk.zig +++ b/src/commands/bulk.zig @@ -73,8 +73,7 @@ pub const Targets = struct { pub fn deinit(self: *Targets) void { self.allocator.free(self.items); if (self.storage) |buf| self.allocator.free(buf); - self.items = &.{}; - self.storage = null; + self.* = undefined; } }; diff --git a/src/config.zig b/src/config.zig index 1fc3098..f968790 100644 --- a/src/config.zig +++ b/src/config.zig @@ -150,6 +150,7 @@ pub const Config = struct { self.allocator.free(entry.value_ptr.*); } self.team_cache.deinit(); + self.* = undefined; } pub fn resolveApiKey(self: *Config, override_key: ?[]const u8) ![]const u8 { diff --git a/src/graphql_client.zig b/src/graphql_client.zig index 3e1faa6..1bf18f0 100644 --- a/src/graphql_client.zig +++ b/src/graphql_client.zig @@ -93,6 +93,7 @@ pub const GraphqlClient = struct { pub fn deinit(self: *GraphqlClient) void { shared_client.release(self.io); + self.* = undefined; } pub const Request = struct { @@ -149,6 +150,10 @@ pub const GraphqlClient = struct { pub fn deinit(self: *Response) void { self.parsed.deinit(); + // Every parsed field is a slice into `parsed`'s arena, so any read + // after this point was already use-after-free. Poisoning makes it + // crash instead of returning plausible bytes. + self.* = undefined; } }; @@ -171,19 +176,19 @@ pub const GraphqlClient = struct { while (true) : (attempt += 1) { response_writer.clearRetainingCapacity(); - if (std.Io.Clock.real.now(self.io).toMilliseconds() >= deadline_ms) return Error.RequestTimedOut; + if (std.Io.Clock.real.now(self.io).toMilliseconds() >= deadline_ms) return error.RequestTimedOut; const attempt_result = try performRequest(self, payload_bytes, &response_writer.writer); rate_limit = attempt_result.rate_limit; const status_code: u16 = attempt_result.status; const after_ms: i64 = std.Io.Clock.real.now(self.io).toMilliseconds(); - if (after_ms >= deadline_ms) return Error.RequestTimedOut; + if (after_ms >= deadline_ms) return error.RequestTimedOut; const can_retry = shouldRetry(status_code) and attempt + 1 < max_attempts; if (can_retry) { const remaining_ms = deadline_ms - after_ms; - const delay_ms = computeDelayMs(attempt, rate_limit, remaining_ms, &random) orelse return Error.RequestTimedOut; + const delay_ms = computeDelayMs(attempt, rate_limit, remaining_ms, &random) orelse return error.RequestTimedOut; logRetry(self.io, status_code, attempt + 2, max_attempts, delay_ms); try self.io.sleep(.fromMilliseconds(@intCast(delay_ms)), .awake); continue; diff --git a/src/tests/main.zig b/src/tests/main.zig index 38a7ee5..54d673f 100644 --- a/src/tests/main.zig +++ b/src/tests/main.zig @@ -9532,6 +9532,7 @@ const FakeProcess = struct { self.calls.deinit(self.allocator); for (self.inputs.items) |input| self.allocator.free(input); self.inputs.deinit(self.allocator); + self.* = undefined; } fn runner(self: *FakeProcess) git.Runner { @@ -12036,10 +12037,12 @@ test "bulk execute records every outcome and keeps running" { const allocator = std.testing.allocator; const Recorder = struct { + const Self = @This(); + seen: *std.ArrayListUnmanaged([]const u8), allocator: std.mem.Allocator, - fn call(self: @This(), index: usize, target: []const u8) !bulk.Outcome { + fn call(self: Self, index: usize, target: []const u8) !bulk.Outcome { _ = index; try self.seen.append(self.allocator, target); return if (std.mem.eql(u8, target, "bad")) .failed else .succeeded; @@ -12501,6 +12504,7 @@ const ChainResult = struct { fn deinit(self: *ChainResult) void { self.cfg.deinit(); self.diagnostics.deinit(); + self.* = undefined; } fn stderrText(self: *ChainResult) []const u8 { @@ -13224,6 +13228,7 @@ const ScratchConfigFixture = struct { self.tmp.cleanup(); restoreEnv(env_name_z, self.saved_key_env, self.allocator); restoreEnv(config_env_name_z, self.saved_config_env, self.allocator); + self.* = undefined; } fn readConfig(self: *ScratchConfigFixture) ![]u8 { diff --git a/src/tests/mock_graphql.zig b/src/tests/mock_graphql.zig index ff45851..25081c8 100644 --- a/src/tests/mock_graphql.zig +++ b/src/tests/mock_graphql.zig @@ -70,6 +70,7 @@ pub const MockServer = struct { entry.value_ptr.*.deinit(); } self.fixtures.deinit(); + self.* = undefined; } pub fn set(self: *MockServer, operation: []const u8, payload: []const u8) !void { @@ -244,6 +245,7 @@ pub const GraphqlClient = struct { pub fn deinit(self: *Response) void { self.parsed.deinit(); + self.* = undefined; } }; @@ -257,7 +259,7 @@ pub const GraphqlClient = struct { } pub fn deinit(self: *GraphqlClient) void { - _ = self; + self.* = undefined; } pub fn send(self: *GraphqlClient, allocator: Allocator, req: Request) !Response { diff --git a/src/tests/online.zig b/src/tests/online.zig index 339b4ca..30e09cd 100644 --- a/src/tests/online.zig +++ b/src/tests/online.zig @@ -31,6 +31,7 @@ const Env = struct { if (self.issue_id) |value| allocator.free(value); if (self.project_id) |value| allocator.free(value); if (self.milestone_id) |value| allocator.free(value); + self.* = undefined; } };