ooprolog fails initial reasoning with 'Contradictory information about merging classes' #211
Comments
|
If you can email |
|
I don't have time to look more right this second, but this is the earliest point where a sanity check fails: |
|
This is the last conclusion:
The arguments are: Rule, VFTableClass, Class, VFTable, Offset, VFTableWrite So basically, instruction 0x747d68 in method 0x747d60 installs VFTable 0x80429c at offset 0. So it makes sense that we would merge 0x747d60 and 0x80429c. |
|
Looking a bit farther back from the problem: What is odd is that the consistency check says "inherits", but the fact says "ObjectInObject". Hmm... |
|
Huh. https://github.com/cmu-sei/pharos/blob/master/share/prolog/oorules/insanity.pl#L66 Ok, that explains the message. And it does seem unlikely that we would inherit at one offset and embed at another. |
|
Offset 0: This is because RTTI tells us so. Offset 0xfc: Hmm... this is because some constructor on 0x748950 calls a constructor 0x747d60 on offset 0xfc.
But 0x747d60 does seem to be a constructor for AVoCViewDocument... |
|
It would be very helpful to see the disassembly for 0x748950 |
|
Okay, here's the disassembly: |
|
Well, it really is calling 0x747d60 on offset 0 and offset 0xfc. I can think of two possibilities:
Here is a godbolt that includes both of these. https://godbolt.org/z/c8Y7ev4Ya The base constructor is called before the vftable is installed. The embedded constructor is called after. Looking at the disassembly above, the constructor at offset 0xfc is called before the vbtable is installed, which would imply that it is inherited (indirectly). But if that is the case, why doesn't the base at offset 0xfc show up in the RTTI? |
|
If you need anything else, just let me know :) I'm a noob when it comes to disassembly/de-compilation and binary analysis so I don't really understand any of this in-depth tbh. So if you need anything you just gotta say so :> |
|
I just turned off the relevant sanity checking rule to see if that would be sufficient, but no, we get another error:
They are indeed not the same class. So why do we think they should be merged? reasonMergeClasses_K says that 0x4e56f0 is callable on both class 0x7f7f58 and class 0x7f7ee4, but neither have base classes, so the two classes must be the same. But judging from the name, it sure sounds like CMusicJingle inherits from CMusicTheme. According to RTTI, that is not so.
Can you provide the disassembly for 0x4e4e30 as well? |
|
It's quite a large function. Here's a gist. |
This is connecting CMusicJingle's VFTable with constructor 0x4e5730. But several other methods also install this VFTable: I suspect that maybe 0x4e5730 is not a constructor for CMusicJingle, but is a constructor for a class that embeds CMusicJingle at offset 0. I will look more at the other methods 0x617690, 0x4e5780, 0x4e76f0, and 0x617700 later. @lmichaelis Can you share this executable with me? This is a complicated problem. I'm not sure that I'll be able to resolve the problem without it. |
|
I've never seen this before; the EXE you sent me actually has debug symbols embedded in it that IDA can read. This makes things a LOT easier! |
I figured. Though Ghidra doesn't seem to be able to find them, weirdly enough. I looked at this file about two / two and a half years ago and back then I remember Ghidra showing hundreds of classes and functions but now it just shows three or so 🤷 |
|
Going back to the original sanity problem where we were inheriting from two offsets, here is the class hierarchy according to IDA: It inherits from CViewDocument once. This means the other object at 0xfc must be embedded. So the assumption we made in the sanity rule is not quite right. That's not a big deal. |
That was wrong. 0x4e5730 is a constructor for CMusicJingle |
|
0x4e4e30 is zCMusicSys_DirectMusic::zCMusicSys_DirectMusic. It's still pretty hard to understand even with symbols! These are not necessarily adjacent lines in the decompilation. So the decompiler is confused about the size of the class. But it does clearly call zCMusicJingle and zCMusicTheme on the same stack offset. There is also no obvious call of zCMusicTheme's destructor. Does zCMusicJingle embed zCMusicTheme (or vice versa)? Or is the compiler somehow reusing stack space for objects with different lifetimes? |
|
I think I got it! https://godbolt.org/z/fz4qhhGTn In this example, the object is not really needed, but the compiler still needs to allocate memory so it can run the constructors. So it reuses the same stack position for both the X and Y objects. This is bad news for the reasonClassRelatedMethod_B rule. |
|
Here's a better example showing the classes don't even need to be related: https://godbolt.org/z/7T4vb9Yc9 |
|
My plan here is to disable reasonClassRelatedMethod_B for objects on the stack (and maybe globals?). We would make it into a guess for these objects. @sei-ccohen Is there a way to detect in prolog whether a thisptr points to a stack object? Or do we need to export more facts for that? A small issue is that reasonClassRelatedMethod_B is that it is a trigger rule, so turning it into a guessing rule could harm performance. |
|
ThisPtrAllocation facts...
https://github.com/cmu-sei/pharos/blob/af54b6ada58d50c046fa899452addce80e9ce8da/share/prolog/oorules/facts.pl#L123
…On Sun, Feb 13, 2022, 11:30 AM sei-eschwartz ***@***.***> wrote:
My plan here is to disable reasonClassRelatedMethod_B for objects on the
stack (and maybe globals?). We would make it into a guess for these objects.
@sei-ccohen <https://github.com/sei-ccohen> Is there a way to detect in
prolog whether a thisptr points to a stack object? Or do we need to export
more facts for that?
A small issue is that reasonClassRelatedMethod_B is that it is a trigger
rule, so turning it into a guessing rule could harm performance.
—
Reply to this email directly, view it on GitHub
<#211 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATGEOB2PVXNS5KQJO27IHDU27MCXANCNFSM5N5HCWPA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Seems like a problem :-) |
|
@sei-ccohen Just a reminder that you are still looking into why thisPtrAllocation facts are not present for stack objects (I think) |
|
@lmichaelis It is not done, but I have a branch where I've been working on this. I've been able to run through the binary and get some results. Please take a look and let us know if there are any obvious problems. |
|
Hm, loading the JSON file into Ghidra using Kaiju produces really weird results. I've picked Before loading the JSON file, there are basically no classes recognized:
After loading it, classes are shown but
Additionally, there seem to be duplicated constructor definitions. According to the analysis It also seems to find waaay too many members: |
|
Thanks, I'll look into this. Definitely sounds like it merged way too many methods into zSTRING. |
|
If I was better with Hex-Rays I would just output all the names of the methods. Maybe I'll try that later. I think that these are all legitimately zSTRING methods: 0x402cd0, 0x41daf0, 0x460a00, 0x46bc00, 0x46c090, 0x46c190, 0x46c290, 0x46c390, 0x46c4e0, 0x46ded0, 0x46e010 There are a whole bunch of constructors for zSTRING that take non-strings as args and convert to string. There are a lot of methods that are NOT really on zSTRING that are merged though. For example, Here is the relevant log: |
|
I added a new rule that is very simple. It says that if a method install the same VFTable at two offsets, then the VFTable's object is contained at both offsets. As a result, the method and VFTable can't be on the same class. This seems pretty obvious and I'm not sure why we didn't have this as a rule. Maybe because we didn't always have VFTable's as a separate object, we couldn't express this in the past and then just never added it? Anyway, running again to see if this helps. |
|
It seems to help on quite a few of those methods, but not all. 0x74d8c0 is the first example I've seen that the new rule does not apply to. |
I can honestly say that I hadn't considered this rule before, so this is a very interesting NEW idea for me. I thought for a minute that we might have a problem with multiple inheritance of the same class twice (once directory and once through another class where the duplicate class was a base) e.g. https://godbolt.org/z/69KMPGEdd But it turns out that we're fine in that case, because the the table ends up being the version for the dervied class (E vftable for C). So yeah, I think this is a great new rule! |
|
So for some reason this had some really terrible side-effects and merged even more things into zSTRING! Unfortunately that run did not have the debug logging I need, so I need to rerun it. |
|
One such method is 0x49eb80: oCVisFX_Lightning::UpdateBurnVobs Proposing guessClassRelatedMethod_A(function=0x49eb80, class1=0x488f50, class2=0x488080, method1=0x488f50, method2=0x488db0). 0x49eb80 is oCVisFX_Lightning::UpdateBurnVobs So that doesn't seem too unreasonable. It's hard to tell where merging has gone wrong. I am attaching an export from IDA of the names of functions. With a little work we can hopefully turn this into a .ground file which will help me debug. I used the following script to dump this: |
That will dump in a format our ooanalyzer-symbolizer tool can read. |
|
Something like this is working fairly well now: I just need to find some time to work backwards to figure out where things start going wrong. |
|
So this is odd. 0x80b1d8 is a RTTI complete object locator, not a method. |
|
Seems to be related to |
|
And The vftable is actually installed at 0x42c1e9, and appears to be at offset 4 of the function's argument. I think I know what is happening. We recently allowed OOAnalyzer to output information about vftable installs that are not in the current object. My guess is that somewhere in reasonVFTableEntry or reasonVirtualFunctionCall we are lacking a check to ensure that the vftable is being installed to the "this" object. |
|
I am looking at this again now. I think the root of the problem is related to the vftable installs. The |
|
AFAICT, the root problem right now is that we are guessing that 0x469790 ?IsActive@zCPlayerInfo@@QAEHXZ is a destructor. It does sort of look like a destructor, except that it returns a value. |
|
Sorry for the long delay. I've been on vacation for most of the last 3 weeks. I'm trying to make sense of what I was doing last. I think 0x469790 may have been guessed by guessFinalRealDestructor, which is basically a very highly speculative guess. This causes a whole bunch of downstream problems. After disabling guessFinalRealDestructor, I was looking at |
|
@lmichaelis Can you take a look at these? I ended up disabling (for now) some of the last guesses we make, which is what I think was causing the problems. |
|
The number of methods looks fine now, though they are all marked as constructors now:
Some definitions appear twice, probably because of that (see
I've caused quite a stir, haven't I? I hope it's not causing you too much trouble :) |
|
Thanks again. I will look into these. I looked only at the
Not at all. It's very useful to see real-world use cases that demonstrate bugs in our rules. Your executable has been very fruitful, showing that several of our rules are messed up. The fact that it also has debug symbols is also really nice for understanding what is going on. |
|
I had a quick look at this. Here is the There are a bunch of constructors, but some are clearly not, such as 0x46bc00 Anyway, OOAnalyzer did not decide that 0x46bc00 is a constructor. Is it appearing in Ghidra as a constructor? If so that is a bug. If you were just commenting that there are "a lot" of constructors in The double destructors in |
|
Oh! Sorry, I didn't realize you had asked a question :/ I did not flag |
Hi there,
while trying to analyze an old binary from 2001 I've come across an error during Prolog analysis. This error persists across multiple fresh retries and I have no idea as to why or how:
This happened to me with the Pharos docker container (digest
sha256:d466e7bb132941e79fd8bebc549a357fc640bf167fa24eadafd7163cfc3ddb16from Feb. 7th) as well as a build from source withcmu-sei/pharos:masterataf54b6ada58d50c046fa899452addce80e9ce8da,rose-compiler/roseataf1323b417efbcdc162b54b667bd0cce4f23be73,swi-prolog/swipl-develat7fc957f2bad34b97efb65357f545eec684bd8a93as well asZ3Prover/z3at30e7c225cd510400eacd41d0a83e013b835a8ece.On the first try I performed analysis using these commands:
partition --serialize=gothic.exe.part --maximum-memory=12000 --threads=8 ../gothic.exe ooanalyzer --serialize=gothic.exe.part --maximum-memory=16000 --prolog-facts=gothic.exe.facts --threads=8 --per-function-timeout=60 --new-method=0x0055D8C0 --new-method=0x00780660 --delete-method=0x0055D8D0 --delete-method=0x0055D8F0 --delete-method=0x0078066F ../gothic.exe ooprolog --facts gothic.exe.facts --results gothic.exe.results --log-level=6 | tee ooprolog.logon another try I added
--partitioner=roseand--no-semanticsin an effort to make it work:partition --serialize=gothic.exe.part --maximum-memory=12000 --threads=8 --no-semantics --partitioner=rose ../gothic.exe ooanalyzer --serialize=gothic.exe.part --maximum-memory=16000 --prolog-facts=gothic.exe.facts --threads=8 --no-semantics --per-function-timeout=60 --new-method=0x0055D8C0 --new-method=0x00780660 --delete-method=0x0055D8D0 --delete-method=0x0055D8F0 --delete-method=0x0078066F ../gothic.exe ooprolog --facts gothic.exe.facts --results gothic.exe.results --log-level=6 | tee ooprolog.logBoth died with the error mentioned above. I can send you the
gothic.exe.factsandooprolog.logfiles via e-mail, if required.Thanks :>
The text was updated successfully, but these errors were encountered: