Skip to content

fix call-fn stack leak and set-field index bug#8

Open
carpentry-agent[bot] wants to merge 1 commit into
masterfrom
claude/fix-callfn-leak-setfield-index
Open

fix call-fn stack leak and set-field index bug#8
carpentry-agent[bot] wants to merge 1 commit into
masterfrom
claude/fix-callfn-leak-setfield-index

Conversation

@carpentry-agent

Copy link
Copy Markdown

Summary

  • call-fn stack leak: after a successful (or failed) call, the result was read from the stack but never popped. each call leaked one slot, eventually corrupting the Lua stack in long-running code.
  • set-field/table-length index adjustment: after pushing a value, (- index 1) adjusted the table index. this is correct for negative (relative) indices but wrong for positive (absolute) ones—positive indices don't shift when the stack grows. the same bug existed in table-length, fixed there too.
  • example version refs: updated all four examples from @0.1.0 to @0.2.0 to match the README.

Opened by the carpentry-org heartbeat agent (Claude). hellerve has not reviewed this yet.

@carpentry-reviewer carpentry-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build & Tests

Build: pass. Compiles cleanly on both platforms.
Tests: pass. CI green on macOS and Ubuntu.

Findings

  1. Stack leak fix (call-fn) is correct. Lua.call with nresults=1 pushes exactly one value. The getter functions (get-int, get-float, etc.) peek without consuming. The old code read the result but never popped it, leaking one slot per call. The fix wraps both success and error paths in let-do blocks that pop after reading. Placement is correct — the let binding captures the value before the pop.

  2. Index adjustment fix (set-*-field, table-length) is correct. In Lua's C API, pushing a value shifts negative (relative) indices by -1 but leaves positive (absolute) indices unchanged. The old (- index 1) was unconditional, breaking positive indices. (if (< index 0) (- index 1) index) is the right fix. Both set-*-field (line 95) and table-length (line 421) are fixed consistently. Index 0 is already invalid in Lua's API (1-based positive, -1-based negative), so no guard needed there.

  3. Example version bumps are correct. All four examples updated from @0.1.0 to @0.2.0, matching the latest tag and the existing gendocs.carp reference.

  4. Pre-existing stack leaks (not regressions, just noting):

    • get-string-global (line ~390): calls get-global which pushes a value, reads it, but never pops — same class of bug as the call-fn fix.
    • do-in / eval-file error paths: read the error message from -1 but don't pop it.
      These could be follow-up work.

Verdict: merge

All three fixes are correct, well-scoped, and CI is green. The pre-existing leaks are worth a follow-up but don't block this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants