Skip to content

fix(gnovm): reject make(chan T) at preprocess time#5238

Open
davd-gzl wants to merge 17 commits intognolang:masterfrom
davd-gzl:fix/reject-make-chan-at-preprocess
Open

fix(gnovm): reject make(chan T) at preprocess time#5238
davd-gzl wants to merge 17 commits intognolang:masterfrom
davd-gzl:fix/reject-make-chan-at-preprocess

Conversation

@davd-gzl
Copy link
Member

@davd-gzl davd-gzl commented Mar 5, 2026

fix #5233

make(chan T) previously passed deployment but panicked with 'not yet implemented' at runtime, leaving a implicit panic on-chain. Reject it during preprocessing instead, producing a clean deployment error.

make(chan T) previously passed deployment but panicked with 'not yet
implemented' at runtime, leaving a permanent landmine on-chain. Reject
it during preprocessing instead, producing a clean deployment error.

Closes gnolang#5233
@Gno2D2 Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Mar 5, 2026
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Mar 5, 2026

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
  • The pull request description provides enough details (checked by @thehowl)
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)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: davd-gzl/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🟢 User MikaelVallenet already reviewed PR 5238 with state APPROVED
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission
The pull request description provides enough details

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])

Can be checked by

  • team core-contributors

@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

❌ Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/preprocess.go 0.00% 3 Missing ⚠️
gnovm/pkg/gnolang/uverse.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Member

@MikaelVallenet MikaelVallenet left a comment

Choose a reason for hiding this comment

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

Implem looks OK, however it's just a runtime panic like a user could throw an explicit one, it's not security issue.
Still IMO it makes sense to block at deployment since there is no expected usecase from this.

@Gno2D2 Gno2D2 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Mar 5, 2026
Copy link
Contributor

@Villaquiranm Villaquiranm left a comment

Choose a reason for hiding this comment

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

LGTM

@MikaelVallenet MikaelVallenet moved this from Waiting for Internal Review (PR) to Ready For Core Review (PR) in Security Bugs - HackenProof Workflow Mar 6, 2026
} else if fv.PkgPath == uversePkgPath && fv.Name == "attach" {
// reserve attach() so we can support it later.
panic("attach() not yet supported")
} else if fv.PkgPath == uversePkgPath && fv.Name == "make" {
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to reject channels at preprocess time, I think the scope should be broader than just make(chan T). Any reference to a channel type could be caught early, for example:

package main

func ch1() chan int {
	return nil
}

func main() {
	var ch chan int
	_ = ch1()
	println(ch)
}

This deploys fine today even though channels aren't supported. IMO if we want a preprocess-time error, it should trigger when the chan type itself is encountered, not only on make. That way var ch chan int, func() chan int, type C chan int, etc. would all be rejected consistently, rather than only catching the one specific case of constructing a channel via make.

omarsy added a commit to omarsy/gno that referenced this pull request Mar 21, 2026
Reject ChanTypeExpr during preprocessing so all channel type usage
(variable declarations, named types, function signatures, struct fields,
make calls) is caught at deployment time with a clear error instead of
panicking at runtime.

Alternative to gnolang#5238 with broader scope — rejects the channel type
itself rather than only make(chan T).

Fixes gnolang#5233
omarsy added a commit to omarsy/gno that referenced this pull request Mar 22, 2026
Reject ChanTypeExpr during preprocessing so all channel type usage
(variable declarations, named types, function signatures, struct fields,
make calls) is caught at deployment time with a clear error instead of
panicking at runtime.

Alternative to gnolang#5238 with broader scope — rejects the channel type
itself rather than only make(chan T).

Fixes gnolang#5233
@thehowl
Copy link
Member

thehowl commented Mar 23, 2026

I suggest we change this PR so that at preprocess time, whenever we process a type expression and it is a chan, we just panic, so we also reject files of this kind:

package main

func main() {
	type T chan int
	var ch chan int
	_ = ch
	_ = T(nil)
	println("ok")
}

Reject ChanTypeExpr during preprocessing so all channel type usage
(variable declarations, named types, function signatures, struct fields,
make calls) is caught at deployment time with a clear error instead of
panicking at runtime.

Broadens the previous make(chan T)-only check per reviewer feedback
from omarsy and thehowl.
@davd-gzl
Copy link
Member Author

I've applied the recommended fix: f15edb2 @thehowl @omarsy
Thank you, it's indeed much simpler / better

@davd-gzl davd-gzl requested a review from omarsy March 24, 2026 06:48
davd-gzl and others added 2 commits March 24, 2026 19:32
The addpkg_private and addpkg_public integration tests declared
a var savedChannel chan *avl.Tree that is now rejected at preprocess
time after rejecting all channel type expressions.
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Mar 24, 2026
Copy link
Contributor

@ltzmaxwell ltzmaxwell left a comment

Choose a reason for hiding this comment

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

Currently, the preprocess stage disallows the chan type, but there are still some occurrences at runtime, which looks inconsistent. It's acceptable as is, but need a proper comment somewhere maybe in preprocess.go to document the current situation and future plan.

davd-gzl and others added 6 commits March 25, 2026 16:50
Co-authored-by: ltzmaxwell <ltz.maxwell@gmail.com>
Address review feedback from ltzmaxwell: document the current situation
where ChanTypeExpr is rejected at preprocess time but still has cases
in other functions. Added explanatory comments and consistent panic
messages ('channel type is not yet supported') in:

- preprocess.go: findUndefinedT and tryPredefine functions
- transcribe.go: transcribe function
- uverse.go: make builtin

Also updated all channel-related filetests to match the new error message.
Copilot AI review requested due to automatic review settings March 26, 2026 05:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses gnovm issue #5233 by rejecting channel types (e.g. make(chan T) and chan T in declarations/signatures) during preprocessing, preventing “not yet implemented” runtime panics from being deployed on-chain.

Changes:

  • Add preprocess-time rejection for ChanTypeExpr, aligning runtime behavior with a clean deployment/preprocess error.
  • Update runtime make() handling to emit a consistent “channel type is not yet supported” message as a backstop.
  • Adjust/add filetests and integration fixtures to avoid/cover channel-type usage.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
gnovm/pkg/gnolang/preprocess.go Rejects ChanTypeExpr during preprocessing; adds/adjusts related handling paths.
gnovm/pkg/gnolang/uverse.go Updates make() chan-type runtime backstop panic message.
gnovm/pkg/gnolang/transcribe.go Adds ChanTypeExpr handling, but currently panics inside traversal (see comments).
gnovm/tests/files/chan_make0.gno New filetest covering make(chan int) rejection.
gnovm/tests/files/chan_type{0,1,2,3}.gno New filetests covering chan usage in var/type/sig/struct-field contexts.
gnovm/tests/files/recurse0.gno Removes channel fields from a recursion/type-printing test and updates golden output.
gnovm/tests/files/extern/net/http/http.gno Removes CloseNotifier interface due to <-chan return type.
gnovm/tests/files/assign_unnamed_type/unnamedtype7_filetest.gno Reworks test away from channels to a pointer-based unnamed-type case.
gnovm/pkg/benchops/gno/opcodes/opcode.gno Updates benchmark/opcode expectations and example struct away from chan types.
gno.land/pkg/integration/testdata/addpkg_{public,private}.txtar Removes saved channel fields from integration test fixtures.

@davd-gzl davd-gzl requested a review from ltzmaxwell March 26, 2026 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

[gnovm] make(chan T) passes deployment but panics at runtime

8 participants