chore: errdefer audit for allocator cleanup on OOM paths - #169
Conversation
Systematic audit of alloc-then-hand-off patterns where an OOM in a later step of the same function would leak an earlier allocation. script_scanner.zig - scanDir / scanPluginDir / scanZigFilesRecursive: a per-file `dupe`'d name (and `allocPrint`'d rel_path) leaked if the `addEntryWithPath` append OOM'd. Added `errdefer` on each. - scanDir / scanPluginDir state-dir branch: `subdir_name` (dupe) and `dir_states` (owned slice from parseDirStates) leaked if the shared-list append OOM'd. Reworked to reserve list capacity before the dupe so the appends are infallible; an explicit `transferred` flag scopes the errdefers to the pre-handoff window. - scanPluginDir: `name_dup` leaked if shared_plugin_names append OOM'd (gemini PR #73 finding) — reserve-then-appendAssumeCapacity. - parseDirStates: a mid-loop dupe/append OOM leaked already-duped state strings + the list buffer. Added an errdefer that frees both. - getEntriesForState: the `result` ArrayList leaked on append OOM. Added `errdefer result.deinit`. - iter.next() errors were swallowed by `catch return`, silently truncating the script list. Now propagated; ScanError folds in std.Io.Dir.Iterator.Error. main_zig.zig - generateMainZigFromTemplate: each emitted block was `toOwnedSlice`'d then `allocs.append`'d; an OOM in that append leaked the block. Reserve allocs capacity up front and use appendAssumeCapacity for all 18 sites, closing the window. root.zig audited — no genuine leaks (rgba_path_allocs already has a matching errdefer; loaded_manifests already reserves capacity). Tests: added two checkAllAllocationFailures-based regression tests exercising every OOM point in scanDir and scanPluginDir. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Script scanning now propagates directory iterator errors (and widens Reviewed by Cursor Bugbot for commit 1e7ae56. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Code Review
This pull request addresses memory leak vulnerabilities and improves OOM robustness across the codebase, specifically targeting issue #75. Key modifications include the use of ensureUnusedCapacity and appendAssumeCapacity to ensure infallible ownership transfers, the addition of errdefer blocks for proper cleanup during partial allocation failures, and the propagation of directory iteration errors. Feedback was provided regarding a hardcoded capacity constant in src/main_zig.zig, which is considered fragile and could lead to runtime panics if new blocks are added without updating the count.
| // is a safe upper bound. Reserving makes the appends infallible, | ||
| // closing the OOM window where a `toOwnedSlice`'d block is owned but | ||
| // not yet in this cleanup list (errdefer audit, #75). | ||
| const ALLOCS_BLOCK_COUNT = 18; |
There was a problem hiding this comment.
The hardcoded constant ALLOCS_BLOCK_COUNT = 18 is fragile and poses a maintainability risk. If a developer adds a new code block and forgets to increment this value, the program will panic at runtime (in debug/release-safe modes) when appendAssumeCapacity is called. While this approach closes the OOM window, consider using ensureUnusedCapacity(allocator, 1) immediately before each toOwnedSlice and append pair to make the code more robust to future changes.
Closes #75
Systematic
errdeferaudit ofscript_scanner.zig,main_zig.zig, androot.zigfor the alloc-then-hand-off pattern where an OOM in a later step of the same function leaks an earlier allocation.Leaks found and fixed
src/script_scanner.zigscanDirroot-file branch (name_copy~L126) — thedupe'd name backs bothfilenameandrel_pathon the entry; leaked ifaddEntryWithPath'sentries.appendOOM'd. Addederrdefer self.allocator.free(name_copy).scanDirstate-dir branch (subdir_name/dir_states~L140-143) —subdir_name(dupe) anddir_states(owned slice fromparseDirStates) leaked if eithershared_subdirs/shared_statesappend OOM'd. Reworked toensureUnusedCapacitybefore the dupe so the appends are infallible; atransferredflag scopes the errdefers to the pre-handoff window.scanPluginDirname_dup (~L178-179) — gemini PR feat(plugins): Controller discovery + ship_from_plugin + two-block scripts #73 finding.name_dupleaked ifshared_plugin_names.appendOOM'd. Reserve-then-appendAssumeCapacity.scanPluginDirfile branch (name_copy/rel_path~L203-205) — gemini findings ~198/~202.name_copyleaked if therel_pathallocPrintOOM'd; both leaked ifaddEntryWithPathOOM'd. Addederrdeferon each.scanPluginDirstate-dir branch (subdir_name/dir_states~L214-216) — gemini finding ~213. Same fix asscanDir's state-dir branch.scanZigFilesRecursivefile branch (name_copy/rel_path~L299-301) — same alloc-then-handoff leak as the plugin file branch. Addederrdeferon each.parseDirStates(~L320) — a mid-loopdupe/appendOOM leaked every state string duped so far plus theArrayListbuffer. Added anerrdeferfreeing both.getEntriesForState(~L272) — theresultArrayListbacking buffer leaked onappendOOM. Addederrdefer result.deinit.iter.next(io) catch returninscanDir/scanPluginDir/scanZigFilesRecursivesilently truncated the script list on a real I/O error (AccessDenied,SystemResources, …). Now propagated viatry;ScanErrorfolds instd.Io.Dir.Iterator.Error.src/main_zig.ziggenerateMainZigFromTemplate(~L2491, 18 append sites) — each emitted code block wastoOwnedSlice'd intoblockthenallocs.append(block)'d; an OOM in that append leakedblock(it never made it into theallocscleanup list). Pre-reserveallocscapacity (ALLOCS_BLOCK_COUNT = 18) and switch all sites toappendAssumeCapacity, closing the window.src/root.zigAudited — no genuine leaks.
rgba_path_allocsalready has a matchingerrdefer allocator.free(rgba_rel);loaded_manifestsalready reserves capacity before itsappendAssumeCapacity(and documents exactly this rationale). Allscanner.*results have matchingdefer freeNames.Tests
Added two
std.testing.checkAllAllocationFailures-based regression tests intest/script_scanner_tests.zig(MemoryLeaksstruct) that drivescanDirandscanPluginDironce per allocation, failing each in turn, and assert no memory leaks on the OOM error path. These fail against the pre-fix code and pass after.Build / test
zig buildandzig build testboth green —18/18 steps succeeded; 411/415 tests passed (4 skipped).🤖 Generated with Claude Code