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

Add pyc plugin #16771

Merged
merged 40 commits into from
May 13, 2020
Merged

Add pyc plugin #16771

merged 40 commits into from
May 13, 2020

Conversation

FXTi
Copy link
Contributor

@FXTi FXTi commented May 5, 2020

Detailed description

Move RBin, RAsm & RAnal plugins for python bytecode from https://github.com/radareorg/radare2-extras

  • Fix .arch field pass in RBin
  • Fix all warnings in compilation
  • Fix header path issue in libr/anal/p/anal_rsp.c

Test plan

test pass

Closing issues

radareorg/radare2-extras#244

@github-actions github-actions bot added the RAnal label May 5, 2020
@radare
Copy link
Collaborator

radare commented May 5, 2020

do not use unnecssary preincrements

Screenshot 2020-05-05 at 18 46 42

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2020

This pull request introduces 5 alerts when merging e0fc42464f070c12623105e076c59736138541c7 into e9152db - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same
  • 1 for FIXME comment

@FXTi FXTi requested a review from radare May 5, 2020 18:38
@lgtm-com
Copy link

lgtm-com bot commented May 5, 2020

This pull request introduces 5 alerts when merging ba4e685af6a9a5de7c2fff8f3df0217a965ebc58 into e9152db - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same
  • 1 for FIXME comment

Copy link
Collaborator

@radare radare left a comment

Choose a reason for hiding this comment

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

The bignum impl should be generic and live in r_util. Use size_t for loop variables. Add some tests

libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode.c Outdated Show resolved Hide resolved
libr/bin/format/pyc/bn.h Outdated Show resolved Hide resolved
libr/asm/p/asm_pyc.c Outdated Show resolved Hide resolved
libr/bin/format/pyc/bn.c Outdated Show resolved Hide resolved
libr/bin/format/pyc/marshal.c Show resolved Hide resolved
@XVilka
Copy link
Contributor

XVilka commented May 6, 2020

Regarding the bignum in r_util - it was discussed long time ago for supporting long registers such as SSE, AVX, etc: https://github.com/radareorg/radare2/issues/6093#issuecomment-312997305
Did time has come?

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2020

This pull request introduces 5 alerts when merging 22bdd557276ba4c16896b74b0935c26c1a49a47b into 4fa630c - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same
  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2020

This pull request introduces 5 alerts when merging 52c59159f1035309642291418d93b6c8750e63de into 64e6df5 - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same
  • 1 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2020

This pull request introduces 5 alerts when merging 19dfafd753925f5ba9401177fd8f4590f2f56276 into 03a5c4d - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same
  • 1 for FIXME comment

@XVilka XVilka added this to the 4.5.0 - Organized Chaos milestone May 7, 2020
@XVilka XVilka requested a review from ret2libc May 7, 2020 04:34
@xarkes
Copy link
Contributor

xarkes commented May 7, 2020

As it's been said above, it would be great if you could add some unit tests for libr/util/big.c :)

@FXTi
Copy link
Contributor Author

FXTi commented May 7, 2020

@xarkes It's still working in progress. I will mention Ready in commits when it's ready.

@XVilka XVilka added the WIP Work-In-Progress label May 7, 2020
@radare
Copy link
Collaborator

radare commented May 7, 2020 via email

@lgtm-com
Copy link

lgtm-com bot commented May 7, 2020

This pull request introduces 8 alerts when merging 379b51c80c6f0a57f5fe70e24c54ac0c00b1deb5 into 4700810 - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same
  • 2 for Unsigned comparison to zero
  • 2 for FIXME comment

@XVilka
Copy link
Contributor

XVilka commented May 8, 2020

By the way, in the future I recommend to squash this PR in two commits - one adding the new bignum + tests, and another - the pyc plugin itself.

@lgtm-com
Copy link

lgtm-com bot commented May 8, 2020

This pull request introduces 8 alerts when merging 7dfb6cb991a4232aa3694c1128a3b88eb9a7fac6 into 2a4cc15 - view on LGTM.com

new alerts:

  • 4 for Comparison result is always the same
  • 2 for Unsigned comparison to zero
  • 2 for FIXME comment

@ret2libc
Copy link
Contributor

ret2libc commented May 8, 2020

Actually, I would suggest to create two completely different PRs. One for RBig and one for PYC. They are very different things and pretty big on their own, so I would prefer to review/merge first the RBig one and then the PYC. WDYT?

@radare
Copy link
Collaborator

radare commented May 8, 2020 via email

@FXTi FXTi requested review from ret2libc and radare May 9, 2020 06:07
@FXTi
Copy link
Contributor Author

FXTi commented May 9, 2020

Agree with ret2libc. But can we squash this into two commits? like @XVilka said.

@lgtm-com
Copy link

lgtm-com bot commented May 9, 2020

This pull request introduces 9 alerts when merging 0bc7513b86f968109b4aaa013ed51f7f52eab359 into 72f9795 - view on LGTM.com

new alerts:

  • 5 for Comparison result is always the same
  • 2 for Unsigned comparison to zero
  • 2 for FIXME comment

@radare
Copy link
Collaborator

radare commented May 9, 2020

Fix the lgtm issues and lgtm

libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/anal/p/anal_pyc.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode_arg_fmt.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode_arg_fmt.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode_anal.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode_38.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode_37.c Show resolved Hide resolved
@XVilka
Copy link
Contributor

XVilka commented May 12, 2020

You removed dependence, but BN changes are still here. Could you please drop them?

@lgtm-com
Copy link

lgtm-com bot commented May 12, 2020

This pull request introduces 1 alert when merging 446824b into 9f2a98b - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@XVilka XVilka self-requested a review May 12, 2020 07:39
libr/anal/p/pyc.mk Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode.c Show resolved Hide resolved
libr/asm/arch/pyc/opcode.c Show resolved Hide resolved
libr/asm/arch/pyc/opcode.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/opcode.c Show resolved Hide resolved
libr/asm/arch/pyc/pyc_dis.c Outdated Show resolved Hide resolved
libr/asm/arch/pyc/pyc_dis.c Outdated Show resolved Hide resolved
libr/bin/format/pyc/pyc.c Outdated Show resolved Hide resolved
libr/bin/p/bin_pyc.c Show resolved Hide resolved
libr/bin/p/bin_pyc.c Show resolved Hide resolved
@FXTi FXTi requested review from wargio and radare May 12, 2020 14:53
libr/bin/format/pyc/marshal.c Outdated Show resolved Hide resolved
libr/bin/format/pyc/marshal.c Outdated Show resolved Hide resolved
@FXTi FXTi requested a review from wargio May 13, 2020 00:37
@XVilka XVilka merged commit 1ffdedc into radareorg:master May 13, 2020
@XVilka
Copy link
Contributor

XVilka commented May 13, 2020

I went ahead and merged it. Please send future improvements in a separate PR. This one got too long.

@FXTi FXTi deleted the pyc branch May 13, 2020 07:39
Emi1305 pushed a commit to Emi1305/radare2 that referenced this pull request Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RAnal WIP Work-In-Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants