-
Notifications
You must be signed in to change notification settings - Fork 0
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
Detect silent failures (no REVERT or error message) #4
Conversation
f06ee8e
to
4c6d403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It frightens me that the index into stack data is calculated from variables of different origins and not bounds checked.
Are there any unit tests in this repo that could test the new behavior?
stack := scope.Stack | ||
for i := l.depth - (depth - 1); l.depth > depth-1; l.depth, i = l.depth-1, i-1 { | ||
if l.current.error == nil { | ||
switch stack.Data()[len(stack.Data())-i].Bytes32()[31] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit hard for me to follow the two loop iterators in my head. If I understand correctly, we always have i == l.depth - (depth - 1)
, right? Maybe instead of tracking i
, we can calculate
stackIndex := len(stack.Data()) - (l.depth - (depth - 1))
on each iteration.
This would also allow us to make a bounds check on the variable instead of crashing out-of-bounds if for some reason the three variables scope.Stack
, depth
and l.depth
get out of sync. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what do we do when it happens?
NOT crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making the code clean enough such that the fatal error that is raised will allow some insight into what went wrong. I'm not aware of the context in which this is run (the filename says "logger", but this is not logging code), so if you say a fatal crash is appropriate so be it.
If you could even include a test, that would be amazing.
@CMajeri is this PR still relevant? Could you link the Jira that tracks the issue? |
https://taurusgroup.atlassian.net/browse/DEV-18502