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.

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

fix: incorrect pointer value comparison #1601

Merged

Conversation

ltzmaxwell
Copy link
Contributor

@ltzmaxwell ltzmaxwell commented Jan 30, 2024

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
Copy link

codecov bot commented Jan 30, 2024

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
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
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: Done
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