frida / frida-tools

Frida CLI tools
Other
348 stars 97 forks source link

New feature: initialize <state> variable with JS scripts before starting trace session #27

Closed mosherubin closed 4 years ago

mosherubin commented 4 years ago

This feature implementation enables initializing the global variable with user-defined functions and variables before the trace session starts. One major use of this feature is to share handler functions across developers, resulting in reusable code.

The feature consists of the -k / --init-session command line options followed by a path to a valid Javascript file. This file will be executed via eval() before the tracing session begins.

See the Git commit message for caveats.

mosherubin commented 4 years ago

Hi Oleavr,

I see it says above "_Add more commits by pushing to the init_sessionimpl branch on mosherubin/frida-tools". I have two more features I am actively using, and would like to return them to Frida master. Do I need to open two additional branches off of master, or, as per the text above, can I just add more commits and pull requests to my branch "init_session_impl"?

oleavr commented 4 years ago

Hey! Thanks for doing this! ❤️ (And sorry for the delay, I've been heads down in unrelated code.) You will have to create each such branch based on upstream master, otherwise the PRs will look very confusing.

mosherubin commented 4 years ago

Regarding always creating a branch off of the upstream master, I thought I was very careful to do so. I even compared the frida/frida-tools master and my own mosherubin/frida-tools master before branching.

Can you tell me if the following web page is the way to keep my fork in sync?

oleavr commented 4 years ago

By the way, it doesn't look like any recent changes made it to the PR. Perhaps the push failed or went to the wrong branch?

mosherubin commented 4 years ago

Hi Oleavr,

OK, I believe I have corrected and pushed the remaining issues.

Regarding not having pushed my changes: I was under the obviously incorrect assumption that I should push my changes only after fixing all of your comments, rather than piecemeal. If I now understand correctly, I can, and probably should, push whenever I fix an issue. Is this correct?

oleavr commented 4 years ago

Can you tell me if the following web page is the way to keep my fork in sync?

That's definitely the safest way to do it, but personally I prefer to rebase instead of merge, to keep the feature-branch history readable. For larger features I try to separate changes into multiple commits, e.g. to separate generic infrastructure additions from the new feature that builds on that infrastructure. I then make use of interactive rebasing (git rebase -i) to refine those commits as the work progresses. The rationale is to keep the history nice and readable for easier review, but also to avoid losing valuable history if upstream feels inclined to squash all of them into one commit at merge-time due to noisy commit history.

oleavr commented 4 years ago

If I now understand correctly, I can, and probably should, push whenever I fix an issue. Is this correct?

Ideally yes, but only if it's not a hassle for you. That way I might get a chance to look at that part of the PR, and resolve that conversation if it looks good, or provide early feedback if there's a misunderstanding.

mosherubin commented 4 years ago

Hi oleavr,

Just to let you know I've been absent from this PR due to an important deadline at work, but hope to return to complete these PRs. I have not forgotten :-)

Moshe

oleavr commented 4 years ago

Tweaked version landed as 8cb6ddae74ced0397264bdb45a832c716c2ab5d5. Thanks! :heart:

oleavr commented 4 years ago

Released in 5.4.0 :tada: