fix(interp): trap negative array.copy size#140
Merged
Conversation
A negative size passed the offset<0 || offset+size>length guard and escaped as a raw Go slice-bounds panic instead of ErrIndexOutOfRange. Guard size < 0 once after popping it, consistent with array.slice's start > end trap. array.fill's negative-size no-op stays as designed. Also simplify I32_OR to Boxed(lhs | rhs) — operands are verified KindI32 so their tags are identical and OR preserves them; only XOR needs the tag-restore formula. Reuse the existing release() builder for the four hand-inlined kept-scan loops in CALL/RETURN_CALL host paths; generated output changes only local variable names.
📝 WalkthroughWalkthroughThe interpreter updates boxed bitwise operation handling, adds negative-size validation for array copies, and refactors host-call reference cleanup in generated and threaded execution paths. A new test covers invalid array-copy parameters. ChangesInterpreter correctness
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Every entry in the lowerings table was named lower<Opcode>; the table already says what they are. Rename to the bare opcode name. CALL, RETURN, and SELECT take an Op suffix: call collides with the fused-call builder, return and select are Go keywords. Generated output is unchanged.
Owner
Author
PR Review Summary — fix(interp): trap negative array.copy sizeStatus: ✅ Merge ReadyTest ResultsAll tests passed:
Merge Readiness AssessmentDecision: Approve This PR is focused, well-scoped, and ready to merge. Scope & Correctness✅ ARRAY_COPY Negative Size Trap
✅ I32_OR Efficiency Optimization
✅ Code Quality Refactoring
Risk Assessment✅ No Breaking Changes
✅ Generated Code Consistency
Readiness Checklist
Minimal Path to Merge: No changes required. Ready to merge once CI completes. Generated by Claude Code |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cmd/geninterp/lower.go (1)
1865-1877: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated HostFunction lowering block into a shared helper.
The entire HostFunction case body (args slice,
fn.Fncall, err check, bothrelease()calls, sp adjustment,copyofout,ip++) is identical betweenlowerCall(1865-1877) andlowerReturnCall(4343-4355). This PR already extractedrelease()for reuse; extending that pattern to the whole block would prevent the two copies from drifting again in the future.♻️ Sketch of a shared helper
+func lowerHostCall() jen.Code { + return jen.Block( + jen.List(jen.Id("fn")).Op(":=").List(jen.Id("fn")), + jen.List(jen.Id("params")).Op(":=").List(jen.Id("len").Call(jen.Id("fn").Dot("Typ").Dot("Params"))), + jen.List(jen.Id("returns")).Op(":=").List(jen.Id("len").Call(jen.Id("fn").Dot("Typ").Dot("Returns"))), + jen.If(jen.Id("i").Dot("sp").Op("<=").Add(jen.Id("params"))).Block(jen.Id("panic").Call(jen.Id("ErrStackUnderflow"))), + jen.If(jen.Id("i").Dot("sp").Op("+").Add(jen.Id("returns")).Op("-").Add(jen.Id("params")).Op("-").Add(jen.Lit(1)).Op(">").Add(jen.Id("len").Call(jen.Id("i").Dot("stack")))).Block(jen.Id("panic").Call(jen.Id("ErrStackOverflow"))), + jen.List(jen.Id("args")).Op(":=").List(jen.Id("i").Dot("stack").Index(jen.Id("i").Dot("sp").Op("-").Add(jen.Id("params")).Op("-").Add(jen.Lit(1)).Op(":").Add(jen.Id("i").Dot("sp").Op("-").Add(jen.Lit(1))))), + jen.List(jen.Id("out"), jen.Id("err")).Op(":=").List(jen.Id("fn").Dot("Fn").Call(jen.Id("i"), jen.Id("args"))), + jen.If(jen.Id("err").Op("!=").Add(jen.Id("nil"))).Block(jen.Id("panic").Call(jen.Id("err"))), + release(jen.Id("args"), jen.Id("out")), + release(jen.Id("i").Dot("stack").Index(jen.Id("i").Dot("sp").Op("-").Add(jen.Lit(1)).Op(":").Add(jen.Id("i").Dot("sp"))), jen.Id("out")), + jen.List(jen.Id("i").Dot("sp")).Op("+=").List(jen.Id("returns").Op("-").Add(jen.Id("params")).Op("-").Add(jen.Lit(1))), + jen.Id("copy").Call(jen.Id("i").Dot("stack").Index(jen.Id("i").Dot("sp").Op("-").Add(jen.Id("returns")).Op(":").Add(jen.Id("i").Dot("sp"))), jen.Id("out")), + jen.Id("i").Dot("fr").Dot("ip").Op("++"), + ) +}Then both
lowerCallandlowerReturnCallcalllowerHostCall()in place of the repeated block.Also applies to: 4343-4355
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/geninterp/lower.go` around lines 1865 - 1877, Extract the duplicated HostFunction case body from lowerCall and lowerReturnCall into a shared lowerHostCall helper, including argument slicing, fn.Fn invocation and error handling, both release calls, stack-pointer adjustment, output copy, and instruction-pointer increment. Replace both inline case bodies with calls to lowerHostCall while preserving their existing behavior and using the established release helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cmd/geninterp/lower.go`:
- Around line 1865-1877: Extract the duplicated HostFunction case body from
lowerCall and lowerReturnCall into a shared lowerHostCall helper, including
argument slicing, fn.Fn invocation and error handling, both release calls,
stack-pointer adjustment, output copy, and instruction-pointer increment.
Replace both inline case bodies with calls to lowerHostCall while preserving
their existing behavior and using the established release helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e0f5c26-b3f9-43d1-a003-1daec2abefa2
📒 Files selected for processing (3)
internal/cmd/geninterp/lower.gointerp/interp_test.gointerp/threaded.go
👮 Files not reviewed due to content moderation or server errors (1)
- interp/threaded.go
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
1 participant
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
ARRAY_COPYwith a negativesizepassed theoffset<0 || offset+size>lengthbounds guard and escaped as a raw Goslice bounds out of rangeruntime panic instead of the VM's typedErrIndexOutOfRange. The generator (lowerArrayCopy) now emits a singleif size < 0trap right after poppingsize, consistent witharray.slice'sstart > endtrap.array.fill's negative-size no-op is deliberate (it compensates ref ownership whensize <= 0) and stays unchanged. No JIT mirror needed —ARRAY_COPYstays threaded perdocs/instruction-set.md.I32_ORsharedI32_XOR's tag-restore formula. Both operands are verifiedKindI32beforeapply(), so their tag bits are identical and OR preserves them directly —I32_ORnow lowers totypes.Boxed(uint64(lhs) | uint64(rhs)), the same shape as the existingI32_ANDcase. All 15 fused sites plus the standalone handler lose two 64-bit ANDs and the masking per execution. XOR keeps the tag-restore formula (XOR zeroes the tag bits).lowerCall/lowerReturnCall's*HostFunctioncases; they now call the existingrelease()builder. Generated output changes only local variable names. No new helpers introduced.All changes are in
internal/cmd/geninterp/lower.go;interp/threaded.gois regenerated output (make generate, verified bymake check-generated).Test plan
TestInterpreter_Runsubtest:ARRAY_COPYwithsize = -1and offsets chosen sooffset+size <= length→ expectsErrIndexOutOfRange(RED before fix:runtime error: slice bounds out of range [2:1]; GREEN after)I32_ORresult value covered by existing test (12 | 10 == 14)make generate+make check-generatedcleango test -race ./...full suite passesmake lintclean🤖 Generated with Claude Code
Summary by CodeRabbit
Follow-up
refactor(geninterp): dropped thelowerprefix from all 210 opcode-handler functions ininternal/cmd/geninterp/lower.go— theloweringstable already names their role.CALL/RETURN/SELECTtake anOpsuffix (callOp,returnOp,selectOp):callcollides with the fused-call builder,return/selectare Go keywords. Generatedinterp/threaded.gois byte-identical.