fix(gnovm): avoid panic on assertion over nil slice#5780
Conversation
🛠 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| if xv.V != nil { | ||
| sv := xv.V.(*SliceValue) | ||
| m.incrCPU(OpCPUSlopeConvertRunesStr * int64(sv.GetLength())) | ||
| } |
There was a problem hiding this comment.
Maybe a philosophical question but should we increment the CPU for converting an empty string (0 length) ?
omarsy
left a comment
There was a problem hiding this comment.
Fix looks correct — a nil slice has length 0, so skipping the incrCPU is gas-neutral, and ConvertTo handles the nil value fine downstream. Two small suggestions inline: use the nil-safe GetLength() accessor instead of guarding the assertion, and consider moving the regression test to gnovm/tests/files/ as a filetest. Neither blocks merging.
[AI] This review was drafted with AI assistance (Claude).
| if xv.V != nil { | ||
| sv := xv.V.(*SliceValue) | ||
| m.incrCPU(OpCPUSlopeConvertRunesStr * int64(sv.GetLength())) | ||
| } |
There was a problem hiding this comment.
Suggestion: TypedValue.GetLength() already handles V == nil (returns 0 for slices), so this can be a one-liner without the assertion:
m.incrCPU(OpCPUSlopeConvertRunesStr * int64(xv.GetLength()))This also mirrors the string→runes branch above, which is nil-safe for the same reason (xv.GetString() returns "" on nil V). Behavior is identical: a nil slice has length 0, so it charges 0 gas either way.
There was a problem hiding this comment.
Done here thanks for the review: 8bec3e7
| @@ -0,0 +1,23 @@ | |||
| gno run . | |||
There was a problem hiding this comment.
Nit on placement: this txtar runs gno run . but lives in testdata/test/, where every other script exercises the gno test subcommand (the testdata subdirs are grouped per subcommand). Since this is a VM-behavior regression rather than a CLI one, the canonical home would be a filetest in gnovm/tests/files/ with an // Output: comment — lighter weight (runs under TestFiles instead of spinning up the CLI), and the issue body is already written in that format, so it converts directly.
Optional while you're at it: only the []MyRune line actually reaches the buggy branch ([]MyByte never enters the Int32Kind gas path). Adding a plain var r []rune; _ = string(r) would document that the bug wasn't specific to declared types.
There was a problem hiding this comment.
Changed here 8bec3e7
For the change on the type I could do it but this file references Golang tests so I don't know if a good idea to modify it let me know if you still want to do the change and I'll do it
closes #5776