fix(gnovm): correct GotoJump stmt-stack truncation for goto out of nested loops#5963
Open
omarsy wants to merge 2 commits into
Open
fix(gnovm): correct GotoJump stmt-stack truncation for goto out of nested loops#5963omarsy wants to merge 2 commits into
omarsy wants to merge 2 commits into
Conversation
…sted loops A backward `goto` that jumps out of three or more nested for/range loops panicked with "slice bounds out of range [:-1]" in Machine.GotoJump. GotoJump reset the statement stack to the outermost popped frame's NumStmts (which already excludes every crossed loop's bodyStmt) and then subtracted depthFrames from it a second time. Once depthFrames exceeded fr.NumStmts — e.g. three loops directly under the function body, where fr.NumStmts == 2 but depthFrames == 3 — the slice index underflowed to [:-1] and panicked. In shallower cases it silently over-truncated, but the GOTO handler in op_exec.go re-truncates m.Stmts to the target block's bodyStmt.NumStmts right after, which masked the bug. Remove the redundant second truncation. Add a regression filetest (goto10.gno) covering a backward goto out of three nested loops, and an ADR.
Collaborator
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
davd-gzl
self-requested a review
July 15, 2026 13:58
davd-gzl
approved these changes
Jul 15, 2026
davd-gzl
left a comment
Member
There was a problem hiding this comment.
Verified on a94c909. Re-adding the deleted second truncation reproduces the slice bounds out of range [:-1] crash. Output matches go run byte-for-byte across for, range, and switch-clause frame crossings for 3- to 6-deep nesting.
notJoon
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a pre-existing interpreter crash: a backward
gotothat jumps out of three or more nestedfor/rangeloops panics within
Machine.GotoJump(gnovm/pkg/gnolang/machine.go). The crash is independent of any gas/profiler work — it reproduces on a clean tree.Reproduction
Before this PR the program panics on the first
goto;go runon the equivalent Go program printsi = 0 / i = 1 / i = 2 / done 10.Root cause
For a
GOTO, the preprocessor recordsFrameDepth(loop/switch-clause frames crossed to reach the label). At runtimeGotoJumpreset the statement stack to the outermost popped frame'sNumStmts:fr.NumStmtsis captured before that frame pushes its ownbodyStmt, and every inner loop pushes later, so line (1) already drops alldepthFramesloopbodyStmts (plus any transient blockbodyStmts) in one shot. Line (2) then subtractsdepthFramesa second time:fr.NumStmts >= depthFrames: silently over-truncates, but theGOTOhandler inop_exec.goimmediately re-truncatesm.Stmtsto the target block'sbodyStmt.NumStmts(re-growing the slice via retained capacity), masking the bug.fr.NumStmts < depthFrames(e.g. three loops directly under the function body →fr.NumStmts == 2,depthFrames == 3): the index underflows to[:-1]and panics.The
m.Blockshandling is not symmetric — after resetting tofr.NumBlocksit pops an additionaldepthBlocks, which is correct becauseBlockDepthcounts scopes within the target frame. Statements have no equivalent second component.The line was introduced in #4219 (which created
GotoJumpto pop loop frames), modeled onPopFrameAndReset's "reset then pop one sticky stmt". That extra pop is valid for a single-frame pop, but wrong when generalized to- depthFramesbecausefrthere is the outermost of many popped frames.Fix
Remove the redundant second truncation.
m.Stmts[:fr.NumStmts]is the correct baseline (consistent with the neighboringOps/Values/Exprs/Blocksresets), andop_exec.goauthoritatively sets the finalm.Stmtslength afterward — so previously-working gotos are unchanged; this is a pure crash fix.Testing
gnovm/tests/files/goto10.gno— panics before the fix, passes after (output verified againstgo run).go test ./gnovm/pkg/gnolang/ -run Files -test.short— 0 failures.goto*/heap_alloc_gotoloop*/loopvar_goto*families pass.go runfor 3- and 4-deep nesting, nestedrangeloops, and gotos additionally crossing block scopes — all match.Files
gnovm/pkg/gnolang/machine.go— remove the erroneous stmt-stack truncation (net one line of logic), with an explanatory comment.gnovm/tests/files/goto10.gno— regression test.gnovm/adr/pr5963_gotojump_nested_loop_stmt_underflow.md— ADR documenting the accounting bug.Developed with AI assistance (Claude Code); all changes were reviewed by the author.