Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: incorrect pointer value comparison #1601

Merged
merged 4 commits into from
Apr 9, 2024
Merged

fix: incorrect pointer value comparison #1601

merged 4 commits into from
Apr 9, 2024

Conversation

ltzmaxwell
Copy link
Contributor

@ltzmaxwell ltzmaxwell commented Jan 30, 2024 •
edited
Loading

This is a fix to #1569 . Thanks to @thehowl for identifying this issue and providing a thorough and insightful analysis.

Here is an further analysis based on #1569 :

package main

func main() {
	{
		b := []byte("ABCDEFGHIJKL")
		a := b
		println(&a[0] == &a[0])
	}
}

this comparison would be false too.

The root cause for this is the way pointer values is obtained and compared, e.g.

package main

func main() {
	c := []byte{'A'}
	a := c
	println(&c[0])
	println(&c[0] == &a[0])
}

in this code snippet, the address of the c[0], (&c[0]) is obtained from this:

    ev := fillValueTV(store, &av.List[ii]) // by reference

that is a reference to the element of the underlying list of arrayValue,

in case like this:

package main

func main() {
	{
		b := []byte("ABCDEFGHIJKL")
		a := b
		println(&a[0] == &a[0])
	}
}

the address is obtained by

	bv := &TypedValue{ // heap alloc, so need to compare value rather than pointer in isEql(), line 482
		T: DataByteType,
		V: DataByteValue{
			Base:     av,
			Index:    ii,
			ElemType: et,
		},
	}

that is a new allocated *TV, which implies the value of it would not be same within the first and second & operation.
So we should actually compare the concrete value rather than the pointers for this case.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jan 30, 2024
@ltzmaxwell ltzmaxwell changed the title fix: incorrect pointer type comparision fix: incorrect pointer type comparison Jan 30, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 30, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.84%. Comparing base (b619351) to head (1bffd37).
Report is 122 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1601      +/-   ##
==========================================
- Coverage   55.80%   45.84%   -9.96%     
==========================================
  Files         436      460      +24     
  Lines       66168    69636    +3468     
==========================================
- Hits        36922    31924    -4998     
- Misses      26356    35052    +8696     
+ Partials     2890     2660     -230

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

@ltzmaxwell ltzmaxwell changed the title fix: incorrect pointer type comparison fix: incorrect pointer value comparison Jan 31, 2024
@deelawn deelawn self-requested a review February 9, 2024 02:20
@deelawn deelawn linked an issue Feb 14, 2024 that may be closed by this pull request
pointers to slice elements in a []byte("string") are != #1569
Closed
deelawn
deelawn suggested changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I think we need to guard against invalid type comparisons.

gnovm/pkg/gnolang/op_binary.go Show resolved Hide resolved
gnovm/pkg/gnolang/values.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/op_expressions.go Outdated Show resolved Hide resolved
@deelawn deelawn mentioned this pull request Feb 28, 2024
Closed
@deelawn deelawn linked an issue Feb 28, 2024 that may be closed by this pull request
println panics on byte pointer types #1575
Closed
@ajnavarro ajnavarro self-requested a review February 29, 2024 17:06
@thehowl thehowl mentioned this pull request Feb 29, 2024
Open
3 tasks
@zivkovicmilos
Copy link
Member

@ltzmaxwell
Can you please check @deelawn's comments on this PR? 🙏

@ltzmaxwell
Copy link
Contributor Author

@ltzmaxwell Can you please check @deelawn's comments on this PR? 🙏

Updated 🙏 .

@deelawn
Copy link
Contributor

deelawn commented Apr 8, 2024

@ltzmaxwell Can you please check @deelawn's comments on this PR? 🙏

Updated 🙏 .

I don't think that fixed it -- see this comment #1601 (comment)

@petar-dambovaliev petar-dambovaliev merged commit ae37e84 into gnolang:master Apr 9, 2024
190 checks passed
@Kouteki Kouteki mentioned this pull request Apr 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petar-dambovaliev petar-dambovaliev petar-dambovaliev approved these changes

@deelawn deelawn deelawn approved these changes

@jaekwon jaekwon Awaiting requested review from jaekwon

@piux2 piux2 Awaiting requested review from piux2

@thehowl thehowl Awaiting requested review from thehowl

@moul moul Awaiting requested review from moul

@ajnavarro ajnavarro Awaiting requested review from ajnavarro

Assignees

@ltzmaxwell ltzmaxwell

Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
💪 Bounties & Worx
Status: Done
🚀 The Launch [DEPRECATED]
Status: No status
🧐 Review Meeting
Status: Done
🧙‍♂️gno.land core team
Archived in project
Milestone
🏗4️⃣ test4.gno.land
Development

Successfully merging this pull request may close these issues.

println panics on byte pointer types pointers to slice elements in a []byte("string") are !=
4 participants
@ltzmaxwell @zivkovicmilos @deelawn @petar-dambovaliev