-
Notifications
You must be signed in to change notification settings - Fork 164
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
PLUGX: make more restrictive to fix FP #843
Conversation
@Still34 FYI |
looks like i need to review the linter failure before this is merged. please review the intent of the logic change in the meantime. |
Shoot, I'll take a look in a bit |
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.
logic LGTM
Looks perfectly fine to me as well |
when I proposed the changes here I imagined (without looking, sorry) that there'd be a large command ID dispatcher that had a bunch of the command IDs referenced in the same function, using a case/switch statement. turns out this is not the case for plugx. instead, we have individual functions that handle each command, parsing or creating the protocol, like: note that we see:
also: along the way i did find a FP in the same sample: see how 0x4000 is used as a bitmask in a related function, not as the command ID: so, i'll propose updating the rule to: - and:
- instruction:
- mnemonic: mov
- operand[0].offset: 0
- or:
- operand[1].number: TIMESTAMP1
- operand[1].number: TIMESTAMP2
- instruction:
- mnemonic: mov
- operand[0].offset: 4
- or:
- operand[1].number: COMMAND1
- operand[1].number: COMMAND2 i think its appropriate to set the scope to basic block, too. |
Throw this timestamp in there as well |
Co-authored-by: Moritz <[email protected]>
I'm not sure why. Maybe we wanted to render it separately... |
closes #842