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

BPF testcases fail due to a bug in PyVEX #13

Closed
zardus opened this issue Jan 14, 2018 · 7 comments
Closed

BPF testcases fail due to a bug in PyVEX #13

zardus opened this issue Jan 14, 2018 · 7 comments
Assignees

Comments

@zardus
Copy link
Member

zardus commented Jan 14, 2018

It looks like BPF support bitrotted between the PR being opened and the PR being merged in. @subwire or @fish, could you take a look?

@zardus zardus assigned zardus, subwire and ltfish and unassigned zardus Jan 14, 2018
@subwire
Copy link
Collaborator

subwire commented Jan 15, 2018

I actually successfully ran the tests before merging. Hmm...
Ninja-edit: I just ran fish's test again and it works locally. Looks like there's an incongruency w/ CI somehow.

@ltfish
Copy link
Member

ltfish commented Jan 15, 2018

I figured out what is going on. Here is the chat log copied from the angr core channel:


fish [1:50 AM]
@lockshaw @subwire
in pyvex/lift/__init__.py:lift()

    for lifter in lifters:
            ...
            next_irsb_part = lifter(arch, addr)._lift(u_data, bytes_offset, max_bytes, max_inst, opt_level, traceflags, allow_lookback)

this is not ideal, since sometimes we have more than one lifters registered (apart from the default VEX lifter), and it's possible that the both lifters are able to lift that instruction

[1:51]
this is exactly why test cases on travis CI is failing: both BPF and MSP430 lifters are currently registered in the CI, and the MSP430 lifter is called first and tries to lift a BPF instruction

[1:55]
here is the old way:

    for lifter, registered_arch in lifters:
        if not isinstance(arch, registered_arch):
            continue

I think the old way is better - we don't accidentally use lifters for one architecture to lift blocks in another architecture

[1:56]
Here is my proposal: we keep a dict that maps each supported architecture to a list of lifters, which will allow us to do the following:

for lifter in arch_lifters[arch.name]:
   ....

what do you think?


EDIT: Fix format issues.

@ltfish
Copy link
Member

ltfish commented Jan 15, 2018

The related PyVEX commit: angr/pyvex@e47ca1b5

@ltfish
Copy link
Member

ltfish commented Jan 15, 2018

To reproduce the issue locally: Add the following line at the beginning of test_bpf_idea.py, right after import angr:

from angr_platforms.msp430 import *

@ltfish ltfish added the bug label Jan 15, 2018
@ltfish ltfish changed the title BPF testcases BPF testcases fail due to a bug in PyVEX Jan 15, 2018
@subwire
Copy link
Collaborator

subwire commented Jan 15, 2018

Now this explains everything.
Yeah, let's add the arch, that makes perfect sense.

@github-actions
Copy link

This issue has been marked as stale because it has no recent activity. Please comment or add the pinned tag to prevent this issue from being closed.

@github-actions github-actions bot added the stale label May 18, 2022
@github-actions
Copy link

This issue has been closed due to inactivity.

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

No branches or pull requests

4 participants