ghidra
ghidra copied to clipboard
NationalSecurityAgency
Fix typo causing "hasRangeMatch" to always be true
It should be set to false initially until a match is found. Because of this error, checkInsideThunkFunction will always think there is a range match, which could cause disastrous results.
Sorry for the delay on this.
We are wondering if you have functionally witnessed an example program where this line has caused problems.
We agree that the line is suspicious, as the value is always true. But the method that contains this line, checkInsideThunkFunction, as well as the the method after it, checkInsideThunkJump, are also in question. We don't know that these methods serve any purpose at this point. They probably were helpful in setting trampoline and thunk functions, but there are separate analyzers that take care of this at this time.
If you look at the applyTo method above these two methods, you will see a sequence of four tests for function == null. The first part of this follows.
Function function = listing.getFunctionAt(address);
if (function == null) {
function = createFunction(program, address);
}
After the above code block has been processed, looking at this code, we cannot fathom any situation where any of the follow-on processing could create a function where the above block could not. In other words, we think the earlier two functions might now be useless.
Have you witnessed a situation where function is still null after the above snippet? What we are interested in is what the circumstances were that caused this? Was the address possibly at the location of an import data table value or was it in External memory? Moreover, did the subsequent processing methods somehow produce a non-null function?
It will take some study to see if we can eliminate some of this logic, but your input would be helpful.
I am not aware of any specific circumstances which triggered this line. However, I will try to see if the mentioned functions continue to serve any meaningful purpose.
It looks like in your last update you mentioned looking into something, and we were just waiting for you to report back.
checkInsideThunkFunction is called when suspected thunk functions are checked to serve no other purpose but to branch to another function after some change has been applied to selected functions, so yes it is still used. @ryanmkurtz
We can see that the call to createFunction(program, address) can return null. And we can see where the checkInsideThunkFunction(program, address) has the issue as noted. So we can see where the creation of a thunk is always attempted inside the try/catch.
If we had had a real example to test against, we posit that Ghidra might produce better results in general with more recent thunk analyzers that kick off after PDB. With such an example we could have done comparisons of the original bad code against your fixed code and that against a modification where both of these calls (checkInsideThunkJump() and checkInsideThunkFunction) are eliminated, allowing the standard thunk analyzers produce those possibly better results.
Since your fix is better than what is there and since this MSDIA-based analyzer's support is waning, we will accept your PR and just won't be able to make the determination of whether we could have done even better than this. Not doing full testing of this, so fingers crossed that there are no bad side effects.