sei-eschwartz
commented
4 years ago Yes, please email them to me if you can't share them publicly.
Closed gynt closed 4 years ago
sei-eschwartz
commented
4 years ago Yes, please email them to me if you can't share them publicly.
sei-eschwartz
commented
4 years ago Thanks, I received them. We will take a look at what is going on. I'm guessing it is related to #135 but we'll see.
sei-ccohen
commented
4 years ago We're pretty sure we've tracked this back to a failure to decide whether 0x479400 is a constructor or destructor but that may not be the root cause. We'll continue investigating. Th eproblem persists with the most recent commit as well. :-(
gynt
commented
4 years ago @sei-ccohen I am happy you are working on it and could reproduce the issue. Does what you are saying mean that I could get it running by passing --new-method=0x479400 ? Or also once with --delete-method= and see what results make most sense?
sei-eschwartz
commented
4 years ago No, new and delete methods are different than constructors and destructors. But as a hack you could try putting factDestructor(0x479400). somewhere in your facts file.
Hopefully we'll have a fix for the bug sometime today.
sei-eschwartz
commented
4 years ago So it looks like 0x479400 is a constructor for std::filebuf. This is very easy to see manually because of the order in which the base class vftables are installed.
There's even a comment in rules.pl that describes detecting constructors and destructors based on this idea:
% Because we already know the direction of the Base/Derived class relationship. This rule is
% complicated. The idea is that knowing which class is the Base and which is the Derived tells
% us whether the Method is a Constructor or Destructor. Ahh... That's probably where this
% rule belongs.But there is no rule for this that I can see. I think such a rule would work well in the presence of RTTI.
Perhaps the more fundamental problem though is that we have enough information to know the method is a constructor or a destructor, but not which one, and we don't have a good way to express this situation and use it to delay.
gynt
commented
4 years ago Such a rule sounds like a good idea.
No, new and delete methods are different than constructors and destructors. But as a hack you could try putting
factDestructor(0x479400).somewhere in your facts file.Hopefully we'll have a fix for the bug sometime today.
Your suggested hack unfortunately does not do the trick:
Entering stage 'Loading data'.
Invalid predicate: factDestructor/1 = factDestructor(0x479400)Try putting it in rules.pl instead
gynt
commented
4 years ago Ha! That worked, I ended up putting factConstructor(0x479400) in rules.pl. The json is about 33% larger than the previous one. The facts file was twice as large as the previous one (the one using --partitioner=rose). Is that a reasonable increase in output (json) given the facts?
sei-eschwartz
commented
4 years ago Yes, that sounds pretty reasonable to me. Glad the hack worked :-)
gynt
commented
4 years ago Wow, the performance difference is great: 203 with the rose partitioner, compared to 289 classes detected now! (FYI)
sei-eschwartz
commented
4 years ago That seems promising. Hopefully the new classes are reasonable, i.e., not just singletons.
gynt
commented
4 years ago How do I best update the Ghidra repo I have? I tried importing the new .json, but that produces new class files, so I have duplicates
sei-eschwartz
commented
4 years ago It's odd that the plugin let you do that... it's not supposed to let you import twice.
Anyway, there is no good way to "update" the results right now. In Ghidra I will use the commit feature to commit before importing and will roll back to avoid re-analysis. But if you have manual notes I'm not sure of a good way to keep them. If that is the case, feel free to file a new issue as it's probably something we should handle.
sei-ccohen
commented
4 years ago Wow, the performance difference is great: 203 with the rose partitioner, compared to 289 classes detected now! (FYI)
I'm thrilled to hear someone (not me) say that! Usually, people just complain that our partitioner extensions just make the partitioning process take too long. :-) The stock ROSE partitioner is much less aggressive above finding and creating code that is only accessed through virtual functions calls. Specifically, the stock ROSE partitioner mostly follows control flow and doesn't make too many "guesses" about where additional code can be. So in my mind, it would make sense that at least in some cases, the results should be better as a consequence of using our extensions.
Having said that, a common OOAnalyzer failure mode is to fail to merge certain methods into the correct class, instead of leaving them on a class of their own, which isn't really a "new" class. If you do some more manual analysis and find that you really are happier with the extra 80-ish classes, I'd like to know that.
sei-eschwartz
commented
4 years ago I believe this is fixed now. If not, please reopen the issue.
Hi all, I am getting a very similar error as reported in #136 :
I do not get the error if I set
--partitioner=rose. I am running the latest version from GitHub (compiled from source). Without--partitioner=rose, I get these warnings and errors as well:Without that option I do not obtain any of the above errors and warnings, but I obtain twice the amount of facts compared to with that option (which makes sense from an accuracy or aggressiveness point of view).
Shall I email my executable and facts file to you @sei-eschwartz at your cert email?