-
Notifications
You must be signed in to change notification settings - Fork 765
fix: fix py27 RAISE_VARARGS_A cause pycdc crash #578
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
base: master
Are you sure you want to change the base?
Conversation
8227fdc
to
8133b2a
Compare
// sometimes JUMP ins after raise | ||
// so we should check the end of current block | ||
// current not in block end, skip 1 ins | ||
if (prev->end() != pos) { | ||
bc_next(source, mod, opcode, operand, pos); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I am not sure I understand why this needs to be handled in
RAISE_VARARGS
opcode and not inJUMP
opcode. - We should only skip
JUMP
opcodes. More interestingly, in the example you have providedJUMP_FORWARD
jumps to the just next instruction and therefore does not seem like it should lead to any crashes! - If this is only for py2.7, we should not perform this logic for all python versions. The changes should be wrapped under a version check.
- Test should be added to avoid future regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I put it in RAISE_VARARGS because of Fix bug in RAISE_VARARGS #561.
- I’m not really sure what the most appropriate handling would be; this is just a mitigation. Since I’m not very familiar with pycdc itself, I haven’t figured out exactly where it should be added.
- Also, I only encountered this issue in Python 2.7, so I’m not sure whether other versions have similar cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thanks, for now let's add some tests with some compiled files. With those I will be more easily able to understand if we need to put it somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thanks, for now let's add some tests with some compiled files. With those I will be more easily able to understand if we need to put it somewhere else.
Ok, I add some test, and I also found some else bug...
After reading the source code, I realized that pycdc doesn’t have the concept of basic blocks. In fact, the root cause of this bug lies in the incorrect handling of dead instructions. However, pycdc doesn’t care about which instructions will be executed and which won’t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I mean pycdc is a decompiler and not an interpreter. Its' goal is to understand the instructions general flow and construct reasonable python code out of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not very familiar with decompilation, but from what I see, decompilers like IDA and Ghidra all build basic blocks, so I think this should be a necessary step in the decompilation process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a broad sense i think pycdc does it, eg try-except blocks or if-else blocks etc. Apart from that I am not sure.
ASTree.cpp
Outdated
curblock->append(prev.cast<ASTNode>()); | ||
|
||
bc_next(source, mod, opcode, operand, pos); | ||
if (prev->end() != pos) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Again as per above, this should be handled in
JUMP
opcode instead of inRAISE_VARARGS
orRETURN_VALUE
. - Test should be added to avoid future regressions.
542a941
to
36e0491
Compare
Please avoid force pushes/rebases. Old comments are marked as outdated otherwise and makes it harder for reviewers to keep track of what changes have been reviewed and what haven't. |
In python 2.7, some times have
JUMP_FORWARD
afterRAISE_VARARGS
, and sometimes not, we should check if we are in the end ofif block
JUMP_FORWARD example:
no JUMP_FORWARD example:
This crash cause by #561