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

Potential nil pointer dereference in x/distribution #5621

Closed
4 tasks
gsora opened this issue Feb 6, 2020 · 5 comments · Fixed by #5620
Closed
4 tasks

Potential nil pointer dereference in x/distribution #5621

gsora opened this issue Feb 6, 2020 · 5 comments · Fixed by #5620

Comments

@gsora
Copy link
Contributor

gsora commented Feb 6, 2020

Summary of Bug

The validateCommunityTax() function implemented here accepts a variable i of type interface{} which is immediately converted to a Dec using a type assertion.

Subsequential call to Dec methods could lead to a nil pointer dereference if i is nil.

Version

Cosmos SDK v0.38

Steps to Reproduce

This Go playground reproduces the issue.

Possible fix

A possible fix could be checking for nil value after the type assertion.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

alexanderbez commented Feb 6, 2020

Indeed true, but when would a Dec or Int parameter ever be nil? We should never allow this. Mainly curious about the code path that allows this.

@gsora
Copy link
Contributor Author

gsora commented Feb 6, 2020

This behavior has been uncovered while updating our app to Cosmos SDK 0.38, mainly due to bad migrations management: the params struct was full of nil fields because we did not execute the migration procedure correctly.

This seems like a corner case which should never happen (migrations should be executed correctly :) but an error would be better than a raw nil pointer dereference-induced panic().

@alexanderbez
Copy link
Contributor

I see. No harm in being extra safe 👌

@alessio
Copy link
Contributor

alessio commented Feb 6, 2020

@alexanderbez The Dec structure cannot be nil. Though the embedded *big.Int could. Attaching Dec structure declaration for reader's convenience:

type Dec struct {
	i *big.Int
}

@alexanderbez
Copy link
Contributor

I already knew that @alessio ;-)

alessio pushed a commit that referenced this issue Feb 7, 2020
Before checking Dec's value, one has to ensure the internal big.Int
pointer is not nil first.

Closes: #5621
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants