fix(gnovm): Prevent use of blank identifier as Value or Type #2699

notJoon · 2024-08-14T14:45:16Z

Description

Closes #1946

The isNamedConversion function now includes a safety check to prevent the use of blank identifiers ("_") as values or types. If both xt and t are nil, the function assumes that a blank identifier is being used inappropriately and panics with an error message that includes the location of the issue.

Variable Explanations

xt (Expression Type): Represents the type of the right-hand side of an assignment or expression. It's the type resulting from evaluating an expression.
t (Target Type): Represents the type of the left-hand side of an assignment. It's the variable or field that will receive the value.

Checks if a named conversion is needed when assigning a value of type xt to a variable of type t.

Preprocess

Added some checks to prevent the disallowd usage of blank identifiers in Preprocess function level. Theses checks are performed at different stages of the preprocessing:

TRANS_ENTER for AssignStmt:
- Checks if both LHS and RHS are blank identifiers in a DEFINE statement.
TRANS_LEAVE for NameExpr:
- Checks if blank identifier is used as a value in disallowed contexts (excluding TRANS_ASSIGN_LHS, TRANS_RANGE_KEY and TRANS_RANGE_VALUE).
TRANS_LEAVE for AssignStmt:
- Checks if RHS is a blank identifier when LHS is not, in a DEFINE statement.

When any of these conditions are met, the function throws an panics like go message.

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-08-14T14:48:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.38%. Comparing base (5c876f3) to head (5d4a2b8).
Report is 59 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2699      +/-   ##
==========================================
+ Coverage   63.18%   63.38%   +0.19%     
==========================================
  Files         561      564       +3     
  Lines       78636    78786     +150     
==========================================
+ Hits        49690    49940     +250     
+ Misses      25569    25469     -100     
  Partials     3377     3377

Flag	Coverage Δ
contribs/gnofaucet	`14.82% <ø> (ø)`
misc/genstd	`79.72% <ø> (ø)`
tm2	`62.39% <ø> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deelawn

Thanks Joon. Looks mostly good as it fixes the example in the issue. Can you also make a change that panics if the RHS identifier is _ during a DEFINE assignment? Maybe assert it is a named expression and compare its name to blankIdentifier if it is.

package main

func main() {
	a := _
}

And while you are making changes regarding the blank identifier, maybe we want to handle this case as well. It is technically a valid expression, though I can't think of a use case, so I'm not sure if we should fix it for any reason other than to more strictly adhere to the go spec.

package main

type zilch interface{}

func main() {
	_ = zilch(nil)
}

It would be good to get @ltzmaxwell 's review on this as well.

notJoon · 2024-08-28T14:27:01Z

@deelawn I've just finished up due to working on other tasks. Now I've modified the code based on your review.

This cases seemed to require handling at the Preprocess function level, so i update that function. Additionally, for the last, case, I also couldn't think of a use case ither, but I thought it would be better to follow the go spec. So I fixed that as well.

ltzmaxwell · 2024-08-29T15:33:07Z

consider this one:

package main

func main() {
	var a = _
}

gnovm/pkg/gnolang/preprocess.go

gnovm/tests/files/typeassert11.gno

ltzmaxwell · 2024-08-29T16:07:29Z

there should be a better PR title too.

notJoon · 2024-09-02T07:04:54Z

@ltzmaxwell I apologize for the delay. I've made some changes to reflect your comments. Still, I can't think of a good idea for the PR title for now. Thanks!

zivkovicmilos · 2024-10-02T09:50:03Z

Pinging @deelawn and @ltzmaxwell for visibility

ltzmaxwell · 2024-10-02T16:41:34Z

gnovm/pkg/gnolang/preprocess.go

+	if xt == nil && t == nil {
+		panic("cannot use _ as value or type")


is there any test targeting this?

d8d53a1

I wrote new test for isNamedConversion function to replace the tests that were previously run through the machine. I checked that this cover everything.

can you provide a file test like this:

package main type ( nat []word word int ) func main() { var a nat b := []word{0} a = b println(a) }

to better illustrate why are these logic(added) needed?
like when xt is nil? when xt and t are both nil?

what I mean here is that I did not find a situation that xt and t are both nil in the context of isNamedConversion.

jefft0 · 2024-10-04T14:05:04Z

Removed the "review team" label because this is already reviewed by core devs.

omarsy

Maybe you should handle the type case:

package main

func main() {
	var i _
	println(i)
}

omarsy · 2024-10-18T19:44:28Z

I think your PR will fix this issue #1461 too

ltzmaxwell · 2024-10-30T13:28:02Z

gnovm/tests/files/typeassert10.gno

@@ -0,0 +1,8 @@
+package main


the name of test files seems to be inaccurate , "typeassert"?

ltzmaxwell · 2024-10-30T13:42:35Z

gnovm/tests/files/typeassert12.gno

+}
+
+// Error:
+// main/files/typeassert12.gno:4:6: cannot use _ as value or type


please consider this also:

package main type S _ func main() { println("hey") }

ltzmaxwell · 2024-10-30T14:03:50Z

gnovm/pkg/gnolang/preprocess.go

@@ -3689,6 +3703,11 @@ func tryPredefine(store Store, last BlockNode, d Decl) (un Name) {
 		})
 		d.Path = last.GetPathForName(store, d.Name)
 	case *ValueDecl:
+		if d.Type != nil {


gno/gnovm/pkg/gnolang/preprocess.go

Lines 501 to 502 in ed91991

if cd, ok := d.(*ValueDecl); ok {

checkValDefineMismatch(cd)

do you think these logics can be integrated to avoid excessive fragmentation?

ltzmaxwell · 2024-10-30T14:07:20Z

gnovm/pkg/gnolang/preprocess.go

+	if xt == nil && t == nil {
+		panic("cannot use _ as value or type")


can you provide a file test like this:

package main type ( nat []word word int ) func main() { var a nat b := []word{0} a = b println(a) }

to better illustrate why are these logic(added) needed?
like when xt is nil? when xt and t are both nil?

ltzmaxwell · 2024-11-18T02:47:47Z

hey @notJoon, do you have time to take a look at the comments, so we can push this forward? thank you.

ltzmaxwell · 2024-11-18T03:05:51Z

gnovm/pkg/gnolang/preprocess.go

 	if t == nil {
 		t = xt
 	}
+	if xt == nil {
+		xt = t
+	}


Prevent use of blank identifier in named conversion

63ccbf3

notJoon requested review from jaekwon, moul, piux2 and deelawn as code owners August 14, 2024 14:45

github-actions bot assigned notJoon Aug 14, 2024

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 14, 2024

notJoon changed the title ~~Fix(gnovm): Prevent use of blank identifier in named conversion~~ fix(gnovm): Prevent use of blank identifier in named conversion Aug 14, 2024

deelawn suggested changes Aug 20, 2024

View reviewed changes

michelleellen mentioned this pull request Aug 21, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

notJoon added 2 commits August 28, 2024 17:46

Merge branch 'master' into fix-1946

e39d2fd

fix preprocess

ea51a76

notJoon requested review from thehowl and mvertes as code owners August 28, 2024 10:46

dongwon8247 mentioned this pull request Aug 28, 2024

Onbloc's Builder Journey gnolang/hackerspace#29

Open

notJoon added 4 commits August 28, 2024 22:20

fix test

714a53d

assign nil value to blank identifier

99a1d1c

lint

441be4e

add example

cfc6512

notJoon requested a review from deelawn August 28, 2024 14:16

ltzmaxwell reviewed Aug 29, 2024

View reviewed changes

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved

ltzmaxwell requested changes Aug 29, 2024

View reviewed changes

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved

ltzmaxwell reviewed Aug 29, 2024

View reviewed changes

gnovm/tests/files/typeassert11.gno Show resolved Hide resolved

fix

9b8f697

notJoon requested a review from ltzmaxwell September 2, 2024 07:04

ltzmaxwell reviewed Oct 2, 2024

View reviewed changes

notJoon added 2 commits October 3, 2024 15:57

Merge branch 'master' into fix-1946

f97c4b2

update test

d8d53a1

notJoon requested a review from ltzmaxwell October 3, 2024 07:54

Kouteki added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 3, 2024

jefft0 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 4, 2024

omarsy reviewed Oct 18, 2024

View reviewed changes

Kouteki added this to the 🚀 Mainnet launch milestone Oct 21, 2024

thehowl and others added 4 commits October 22, 2024 17:53

Merge branch 'master' into fix-1946

cb54594

var decl with blank type

7610983

partial revert

e2cb013

fix

5d4a2b8

Kouteki modified the milestones: 🚀 Mainnet launch, ⏭️Next after mainnet Oct 26, 2024

ltzmaxwell reviewed Oct 30, 2024

View reviewed changes

Kouteki added the in focus Core team is prioritizing this work label Nov 15, 2024

Kouteki requested review from ltzmaxwell and petar-dambovaliev and removed request for jaekwon, moul, deelawn, thehowl and piux2 November 15, 2024 09:40

Kouteki modified the milestones: ⏭️Next after mainnet, 🚀 Mainnet launch Nov 15, 2024

ltzmaxwell reviewed Nov 18, 2024

View reviewed changes

gnovm/pkg/gnolang/preprocess.go

if t == nil {

t = xt

}

if xt == nil {

xt = t

}

Copy link

Contributor

ltzmaxwell Nov 18, 2024

Choose a reason for hiding this comment

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

also this.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gnovm): Prevent use of blank identifier as Value or Type #2699

fix(gnovm): Prevent use of blank identifier as Value or Type #2699

notJoon commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

deelawn left a comment

notJoon commented Aug 28, 2024

ltzmaxwell commented Aug 29, 2024

ltzmaxwell commented Aug 29, 2024

notJoon commented Sep 2, 2024

zivkovicmilos commented Oct 2, 2024

ltzmaxwell Oct 2, 2024 •

edited

Loading

notJoon Oct 3, 2024

ltzmaxwell Oct 30, 2024 •

edited

Loading

ltzmaxwell Nov 18, 2024

jefft0 commented Oct 4, 2024

omarsy left a comment •

edited

Loading

omarsy commented Oct 18, 2024

ltzmaxwell Oct 30, 2024 •

edited

Loading

ltzmaxwell Oct 30, 2024

ltzmaxwell Oct 30, 2024

ltzmaxwell Oct 30, 2024 •

edited

Loading

ltzmaxwell commented Nov 18, 2024

ltzmaxwell Nov 18, 2024

		if xt == nil && t == nil {
		panic("cannot use _ as value or type")

fix(gnovm): Prevent use of blank identifier as Value or Type #2699

Are you sure you want to change the base?

fix(gnovm): Prevent use of blank identifier as Value or Type #2699

Conversation

notJoon commented Aug 14, 2024 • edited Loading

Description

Variable Explanations

Preprocess

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

deelawn left a comment

Choose a reason for hiding this comment

notJoon commented Aug 28, 2024

ltzmaxwell commented Aug 29, 2024

ltzmaxwell commented Aug 29, 2024

notJoon commented Sep 2, 2024

zivkovicmilos commented Oct 2, 2024

ltzmaxwell Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

notJoon Oct 3, 2024

Choose a reason for hiding this comment

ltzmaxwell Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

ltzmaxwell Nov 18, 2024

Choose a reason for hiding this comment

jefft0 commented Oct 4, 2024

omarsy left a comment • edited Loading

Choose a reason for hiding this comment

omarsy commented Oct 18, 2024

ltzmaxwell Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

ltzmaxwell Oct 30, 2024

Choose a reason for hiding this comment

ltzmaxwell Oct 30, 2024

Choose a reason for hiding this comment

ltzmaxwell Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

ltzmaxwell commented Nov 18, 2024

ltzmaxwell Nov 18, 2024

Choose a reason for hiding this comment

notJoon commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

ltzmaxwell Oct 2, 2024 •

edited

Loading

ltzmaxwell Oct 30, 2024 •

edited

Loading

omarsy left a comment •

edited

Loading

ltzmaxwell Oct 30, 2024 •

edited

Loading

ltzmaxwell Oct 30, 2024 •

edited

Loading