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

Fix gymrat disasm #367

Merged
merged 10 commits into from
Nov 30, 2023
Merged

Fix gymrat disasm #367

merged 10 commits into from
Nov 30, 2023

Conversation

xxr0ss
Copy link
Contributor

@xxr0ss xxr0ss commented Nov 25, 2023

The disassemble() doesn't work because GymratLifter._lift() doesn't receive its args, which are not passed through Lifter.lift()

class GymratLifter(Lifter):
    ...
    def _lift(self, disassemble=False, dump_irsb=False):

and disasm result isn't returned from Lifter.lift(), which returns self.irsb instead of self._lift():

class Lifter:
    ...
    def lift(
        self,
        data,
        bytes_offset=None,
        max_bytes=None,
        max_inst=None,
        opt_level=1,
        traceflags=None,
        allow_arch_optimizations=None,
        strict_block_end=None,
        skip_stmts=False,
        collect_data_refs=False,
        cross_insn_opt=True,
        load_from_ro_regions=False,
    )
        ...
        self._lift()
        return self.irsb

I think lift() should return IRSB only because of its sematic, however GymratLifter (as well as many other code not listed here) also uses _lift() to return disassembly. Well, let's allow that.

@twizmwazin
Copy link
Member

I'm not a fan of this solution, instead I would rather make the gymrat options be explicit kwargs on lift() instead and have them set as instance variables to be optionally read in _lift, just like the many libvex-specific args.

@xxr0ss
Copy link
Contributor Author

xxr0ss commented Nov 28, 2023

@twizmwazin I agree. Let's just override the lift()!

@xxr0ss xxr0ss marked this pull request as draft November 28, 2023 18:40
@xxr0ss xxr0ss marked this pull request as ready for review November 28, 2023 19:01
@twizmwazin
Copy link
Member

No, you misunderstand. lift should not be overridden, and _lift is private and should never be called by users of a lifter class. In the base Lifter class, we can add extra kwargs for the gymrat lifter, like how there are already many extra kwargs for the libvex lifter. These extra kwargs can be set on self, and then the _lift in the gymrat lifter can read them.

@xxr0ss
Copy link
Contributor Author

xxr0ss commented Nov 29, 2023

@twizmwazin thanks for the detailed explanation!

@xxr0ss xxr0ss requested a review from twizmwazin November 30, 2023 06:16
@twizmwazin twizmwazin merged commit 63bc840 into angr:master Nov 30, 2023
18 checks passed
@xxr0ss xxr0ss deleted the fix_gymrat branch November 30, 2023 18:56
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.

2 participants