Update is_valid_opcode security#75
Conversation
|
Hey @pipermerriam / @gsalgado still some cleaning and refactoring to be done, but if you don't mind giving this a quick lookover to make sure i'm on the right track/didn't miss anything important, that would be awesome! |
pipermerriam
left a comment
pipermerriam
left a comment
There was a problem hiding this comment.
Bits of feedback. Looks really good in general.
evm/vm/code_stream.py
Outdated
| validate_is_bytes(code_bytes) | ||
| self.stream = io.BytesIO(code_bytes) | ||
| self._validity_cache = {} | ||
| self._validity_cache = set(range(0)) |
There was a problem hiding this comment.
This should probably be changed to just set() as I think the range(0) part is not necessary. And can we now rename this to invalid_positions since it's no longer really a cache.
evm/vm/code_stream.py
Outdated
| self.stream = io.BytesIO(code_bytes) | ||
| self._validity_cache = {} | ||
| self._validity_cache = set(range(0)) | ||
| self.deepest = 0 |
There was a problem hiding this comment.
name nitpick:
Thoughts on renaming to depth_processed to be a bit more explicit?
evm/vm/code_stream.py
Outdated
| i = self.deepest | ||
| while i <= position: | ||
| # get opcode | ||
| with self.seek(i): |
There was a problem hiding this comment.
It'd be nice to do this without seeking into it. Can we just add an __getitem__ to this object so you can just do self[i] to grab the value at the given length?
evm/vm/code_stream.py
Outdated
| # if opcode = pushxx | ||
| if opcode >= opcode_values.PUSH1 and opcode <= opcode_values.PUSH32: | ||
| # add range(xx) to val_cache | ||
| self._validity_cache.update(range((i + 1), ((i + 1) + (opcode - 95)))) |
There was a problem hiding this comment.
For lines of code like this I've found that the following pattern makes a lot better readability
left_bound = ....
right_bound = ....
invalid_range = range(left_bound, right_bound)
self._validity_cache.update(invalid_range)
That way each line only contains a single concept which makes it easier to ingest one piece at a time.
njgheorghita
changed the title
* new issue and pr templates
* new issue and pr templates
2 participants
What was wrong?
Codestream processing was insecure
How was it fixed?
Updated codestream processing algorithm
Cute Animal Picture