Skip to content
This repository was archived by the owner on Jun 24, 2026. It is now read-only.

Symbolic memory model bugfixes#1350

Merged
disconnect3d merged 10 commits into
masterfrom
dev-mem-callbacks
Jan 16, 2019
Merged

Symbolic memory model bugfixes#1350
disconnect3d merged 10 commits into
masterfrom
dev-mem-callbacks

Conversation

@ehennenfent

@ehennenfent ehennenfent commented Jan 15, 2019

Copy link
Copy Markdown
Contributor

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


This change is Reviewable

Comment thread manticore/core/smtlib/expression.py Outdated

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

@disconnect3d disconnect3d Jan 15, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@feliam feliam Jan 15, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

I removed it.


def read(self, name):
name = self._alias(name)
name = str(self._alias(name))

Copy link
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants