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

Symbolic memory model bugfixes #1350

Merged
merged 10 commits into from
Jan 16, 2019
Merged

Symbolic memory model bugfixes #1350

merged 10 commits into from
Jan 16, 2019

Conversation

ehennenfent
Copy link
Contributor

@ehennenfent ehennenfent commented Jan 15, 2019

Handles edge cases that sometimes occur with symbolic memory models. Nothing major, just merging so we can delete the branch.


This change is Reviewable

@@ -308,6 +308,10 @@ def __ror__(self, other):
def __invert__(self):
return BitVecXor(self, self.cast(self.mask))

def __len__(self):
"""Returns the bitvector size, in bytes(!)"""
return self.size // 8
Copy link
Member

@disconnect3d disconnect3d Jan 15, 2019

Choose a reason for hiding this comment

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

@feliam is it fine to return the length of a BitVec in bytes?

Copy link
Contributor

Choose a reason for hiding this comment

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

idk. It is a bit vector.
pyZ3 does not use len but rather a bv.size()

We should carefully check that it is not used for bits anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

@ehennenfent any context why do we need to add this in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure the original reason was to allow this assert to pass when using a memory model that returns a BitVec.

Copy link
Contributor

@feliam feliam Jan 15, 2019

Choose a reason for hiding this comment

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

We need a way to read the size of a bitvec. Maybe using _len is not the best way.
We already have .size

Are we sure we'll never support a 4 bit bitvec?

It may be better if we remove it (__len__).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've since found a way to make the symbolic memory model work without this workaround, so if you think it's better to delete than risk setting ourselves up for failure later, that's fine with me.

Copy link
Member

Choose a reason for hiding this comment

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

I removed it.

@@ -583,7 +583,7 @@ def _update_cache(self, name, value):
self._cache.pop(affected, None)

def read(self, name):
name = self._alias(name)
name = str(self._alias(name))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For context, this prevents an issue on line 591, where Python doesn't like evaluating if 'FLAGS' in name: if name is not a string. I don't remember how I triggered it, but since the name should already be a string in most reasonable cases, I don't think this will break any functionality.

@disconnect3d disconnect3d merged commit 1da68dc into master Jan 16, 2019
@disconnect3d disconnect3d deleted the dev-mem-callbacks branch January 16, 2019 16:04
disconnect3d added a commit that referenced this pull request Feb 12, 2019
* master:
  Ensure native deps (better error message) (#1367)
  Make sys_lseek return offset location (#1355)
  Fix a typo in the documentation (#1360)
  Refactor tests structure (#1352)
  Dev single gas calc (#1353)
  Symbolic memory model bugfixes (#1350)
  Refactor not-existing SValue into BitVecVariable
  Config with context (#1345)
  Update Capstone to 4.0.1 (#1312)
  evm: fix _check_jumpdest when run with detectors (#1347)
  Move tx default gas value to config (#1346)
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 this pull request may close these issues.

3 participants