-
Notifications
You must be signed in to change notification settings - Fork 0
Integrate FridaExtractor into capa and add arguments #4
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
Conversation
mike-hunhoff
left a comment
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.
Nice work, I've left a couple of comments for your review
| "process_id": Process.id, | ||
| "thread_id": Process.getCurrentThreadId(), | ||
| "call_id": call_id, | ||
| "call_id": call_id - 1, |
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.
Why minus 1?
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.
call_id++ happens in recordApiCall(), so when debugLog() is called after it, we need to subtract 1 to get the actual call_id.
debugLog() only used to debug, I just noticed this mistake.
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.
Hmm this could introduce a very subtle bug where the ++ and - 1 become out of sync, resulting in the incorrect call_id value getting logged. Can we pass the appropriate call_id value to this function? Or pass and use information from the API call record?
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.
Oh, you're right. I'll go with passing the call_id value, it is cleaner.
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.
Ah oh~ I found a simpler solution. I can just put debugLog() before recordApiCall() in each hook, and no need to minus 1 now.
recordApiCall() is the actual production code, I think it's better to keep it unchanged.
| var apiCallRecord = { | ||
| "process_id": Process.id, | ||
| "thread_id": Process.getCurrentThreadId(), | ||
| "call_id": call_id++, |
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.
similar to @mike-hunhoff comment below, I'm not very comfortable with the self-increment here. It is very error-prone when program get bigger.
But since this script is going to be auto-generated. We can leave it for now until you get your FridaScript generation script.
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.
Got it now! So the point is to centralize the ID increment management instead of letting any individual method control it.
Done: