Set Default Behavior to Skip Re-Instrumentation of Duplicate Modules#86
Set Default Behavior to Skip Re-Instrumentation of Duplicate Modules#86ifratric merged 9 commits intogoogleprojectzero:masterfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
tinyinst.cpp
Outdated
| if(address) { | ||
| resolved_hooks[address] = hook; | ||
| } else { | ||
| FATAL("Could not resolve function %s in module %s", hook->GetFunctionName().c_str(), hook->GetModuleName().c_str()); |
There was a problem hiding this comment.
We don't want to FATAL() here because hook's module name could be "*" so it searches for the function in all modules.
There was a problem hiding this comment.
Ah that makes good sense. Do you foresee a better way to alert the user if a hook function is unable to be resolved? Or do you think it's not necessary?
There was a problem hiding this comment.
I suppose I could just change it to WARN()
README.md
Outdated
|
|
||
| `-indirect_instrumentation [none|local|global|auto]` which instrumentation to use for indirect jump/calls | ||
|
|
||
| `-ignore_duplicates_module [module name]` Ensures only the first loaded instance of `[module name]` is instrumented, ignoring subsequent duplicates. Useful when instrumenting system libraries (e.g., `CoreAudio`) on macOS Sequoia and later, where multiple distinct libraries with the same name may be loaded. |
There was a problem hiding this comment.
I think having a flag for this is not needed; TinyInst will not currently work correctly with duplicate module names, so the current ignore_duplicates behavior should just be the default implementation for all modules. Just issuing a warning on duplicate module and not instrumenting it is completely sufficient.
Later we need to think how to resolve this e.g. if the user wants to instrument the second module with the same name (or multiple modules with the same name). For that, there should probably be an option to specify instrumented_module by full path, but we can figure that out later.
There was a problem hiding this comment.
Sounds good! I will go ahead and remove the flag and implement ignoring duplicate modules as the default behavior :)
Made skipping re-instrumentation of duplicate modules default behavior
removed duplicate module name addition
ifratric
left a comment
ifratric
left a comment
There was a problem hiding this comment.
Awesome! I left one more comment regarding the hook warning, but otherwise looks good!
| if(address) { | ||
| resolved_hooks[address] = hook; | ||
| } else { | ||
| WARN("Could not resolve function %s in module %s", hook->GetFunctionName().c_str(), hook->GetModuleName().c_str()); |
There was a problem hiding this comment.
I think it only makes sense to issue this warning if hook->GetModuleName() != "*". Could you add this check?
dillonfranke
changed the title
added additional check for wildcard module before warning of failed hook resolution
|
Cool, I think we can merge now! |
2 participants
Uh oh!
There was an error while loading. Please reload this page.