Open lmichaelis opened 2 years ago
sei-eschwartz
commented
2 years ago If you can email gothic.exe.facts to me I will take a look at what is going on. If you can include gothic.exe, that can sometimes be helpful as well, though it is not always necessary.
sei-eschwartz
commented
2 years ago I don't have time to look more right this second, but this is the earliest point where a sanity check fails:
Objects now on 0x80429c: [0x747d60, 0x80429c]
reasonForwardAsManyTimesAsPossible
Consistency checks failed.
Class 0x804320 inherits from 0x80429c at offsets 0 and 0xfc[eschwartz@pd4 issue211]$ cat gothic.exe.results.log | fgrep -e 0x804320 -e 0x80429c | fgrep rTTI
rTTISelfRef(0x87da38, 0x811ba8, 0x811b98, 0x811b58, 0x80429c, '.?AVoCViewDocument@@').
rTTISelfRef(0x87da58, 0x811c48, 0x811c38, 0x811bf0, 0x804320, '.?AVoCViewDocumentMap@@').This is the last conclusion:
reasonMergeVFTables_A(constructor, 0x80429c, 0x747d60, 0x80429c, 0, factVFTableWrite(0x747d68, 0x747d60, 0, 0x80429c)).
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.
sei-eschwartz
commented
2 years ago Looking a bit farther back from the problem:
Concluding mergeVFTables(0x80429c, 0x747d60).
Merging class 0x747d60 into 0x80429c ...
Retracting factClassSizeGTE(0x747d60, 0xf0) and asserting factClassSizeGTE(0x80429c, 0xf0) ...
Retracting factClassSizeLTE(0x747d60, 0xfffffff) and asserting factClassSizeLTE(0x80429c, 0xfffffff) ...
Retracting factNOTMergeClasses(0x747d60, 0x804320) and asserting factNOTMergeClasses(0x80429c, 0x804320) ...
Retracting factNOTMergeClasses(0x747d60, 0x809fd0) and asserting factNOTMergeClasses(0x80429c, 0x809fd0) ...
Retracting factNOTMergeClasses(0x747d60, 0x80a000) and asserting factNOTMergeClasses(0x80429c, 0x80a000) ...
Retracting factObjectInObject(0x804320, 0x747d60, 0) and asserting factObjectInObject(0x804320, 0x80429c, 0) ...
Retracting factObjectInObject(0x804320, 0x747d60, 0xfc) and asserting factObjectInObject(0x804320, 0x80429c, 0xfc) ...
Objects now on 0x80429c: [0x747d60, 0x80429c]
reasonForwardAsManyTimesAsPossible
Consistency checks failed.
Class 0x804320 inherits from 0x80429c at offsets 0 and 0xfcWhat is odd is that the consistency check says "inherits", but the fact says "ObjectInObject". Hmm...
sei-eschwartz
commented
2 years ago 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.
sei-eschwartz
commented
2 years ago Offset 0: reasonDerivedClass_D(0x87da58, 0x87da38, 0x804320, 0x80429c, 0).
This is because RTTI tells us so.
Offset 0xfc: reasonObjectInObject_D(0x748950, 0x747d60, 0xfc)
Hmm... this is because some constructor on 0x748950 calls a constructor 0x747d60 on offset 0xfc.
validMethodCallAtOffset(0x748983, 0x748950, 0x747d60, 0xfc)
But 0x747d60 does seem to be a constructor for AVoCViewDocument...
sei-eschwartz
commented
2 years ago It would be very helpful to see the disassembly for 0x748950
lmichaelis
commented
2 years ago Okay, here's the disassembly:
luis:Gothic1/ $ objdump -d --start-address=0x748950 --stop-address=0x748A82 -M intel gothic.exe [6:27:11]
gothic.exe: file format pei-i386
Disassembly of section .text:
00748950 <.text+0x347950>:
748950: 6a ff push 0xffffffff
748952: 68 44 dc 7e 00 push 0x7edc44
748957: 64 a1 00 00 00 00 mov eax,fs:0x0
74895d: 50 push eax
74895e: 64 89 25 00 00 00 00 mov DWORD PTR fs:0x0,esp
748965: 83 ec 0c sub esp,0xc
748968: 53 push ebx
748969: 55 push ebp
74896a: 56 push esi
74896b: 8b e9 mov ebp,ecx
74896d: 57 push edi
74896e: 89 6c 24 18 mov DWORD PTR [esp+0x18],ebp
748972: e8 e9 f3 ff ff call 0x747d60
748977: 33 f6 xor esi,esi
748979: 8d 8d fc 00 00 00 lea ecx,[ebp+0xfc]
74897f: 89 74 24 24 mov DWORD PTR [esp+0x24],esi
748983: e8 d8 f3 ff ff call 0x747d60
748988: 8a 44 24 13 mov al,BYTE PTR [esp+0x13]
74898c: 88 85 00 02 00 00 mov BYTE PTR [ebp+0x200],al
748992: 89 b5 04 02 00 00 mov DWORD PTR [ebp+0x204],esi
748998: 89 b5 08 02 00 00 mov DWORD PTR [ebp+0x208],esi
74899e: 89 b5 0c 02 00 00 mov DWORD PTR [ebp+0x20c],esi
7489a4: c7 85 fc 01 00 00 ec mov DWORD PTR [ebp+0x1fc],0x7f56ec
7489ab: 56 7f 00
7489ae: 83 c9 ff or ecx,0xffffffff
7489b1: 33 c0 xor eax,eax
7489b3: bf 48 db 87 00 mov edi,0x87db48
7489b8: c7 45 00 20 43 80 00 mov DWORD PTR [ebp+0x0],0x804320
7489bf: c7 45 24 fc 42 80 00 mov DWORD PTR [ebp+0x24],0x8042fc
7489c6: c7 85 ec 00 00 00 f4 mov DWORD PTR [ebp+0xec],0x8042f4
7489cd: 42 80 00
7489d0: 89 b5 f8 01 00 00 mov DWORD PTR [ebp+0x1f8],esi
7489d6: f2 ae repnz scas al,BYTE PTR es:[edi]
7489d8: f7 d1 not ecx
7489da: 49 dec ecx
7489db: 8b f9 mov edi,ecx
7489dd: 83 ff fd cmp edi,0xfffffffd
7489e0: c6 44 24 24 02 mov BYTE PTR [esp+0x24],0x2
7489e5: 8d 9d 00 02 00 00 lea ebx,[ebp+0x200]
7489eb: 89 7c 24 14 mov DWORD PTR [esp+0x14],edi
7489ef: 76 05 jbe 0x7489f6
7489f1: e8 9b 2f 05 00 call 0x79b991
7489f6: 8b 4b 04 mov ecx,DWORD PTR [ebx+0x4]
7489f9: 3b ce cmp ecx,esi
7489fb: 74 1e je 0x748a1b
7489fd: 8a 41 ff mov al,BYTE PTR [ecx-0x1]
748a00: 84 c0 test al,al
748a02: 74 17 je 0x748a1b
748a04: 3c ff cmp al,0xff
748a06: 74 13 je 0x748a1b
748a08: 3b fe cmp edi,esi
748a0a: 75 33 jne 0x748a3f
748a0c: fe c8 dec al
748a0e: 88 41 ff mov BYTE PTR [ecx-0x1],al
748a11: 56 push esi
748a12: 8b cb mov ecx,ebx
748a14: e8 57 aa cb ff call 0x403470
748a19: eb 52 jmp 0x748a6d
748a1b: 3b fe cmp edi,esi
748a1d: 75 0b jne 0x748a2a
748a1f: 6a 01 push 0x1
748a21: 8b cb mov ecx,ebx
748a23: e8 48 aa cb ff call 0x403470
748a28: eb 43 jmp 0x748a6d
748a2a: 8b 43 0c mov eax,DWORD PTR [ebx+0xc]
748a2d: 83 f8 1f cmp eax,0x1f
748a30: 77 04 ja 0x748a36
748a32: 3b c7 cmp eax,edi
748a34: 73 11 jae 0x748a47
748a36: 6a 01 push 0x1
748a38: 8b cb mov ecx,ebx
748a3a: e8 31 aa cb ff call 0x403470
748a3f: 57 push edi
748a40: 8b cb mov ecx,ebx
748a42: e8 89 aa cb ff call 0x4034d0
748a47: 8b 44 24 14 mov eax,DWORD PTR [esp+0x14]
748a4b: 8b cf mov ecx,edi
748a4d: 8b 7b 04 mov edi,DWORD PTR [ebx+0x4]
748a50: 8b d1 mov edx,ecx
748a52: c1 e9 02 shr ecx,0x2
748a55: be 48 db 87 00 mov esi,0x87db48
748a5a: f3 a5 rep movs DWORD PTR es:[edi],DWORD PTR ds:[esi]
748a5c: 8b ca mov ecx,edx
748a5e: 83 e1 03 and ecx,0x3
748a61: f3 a4 rep movs BYTE PTR es:[edi],BYTE PTR ds:[esi]
748a63: 8b 4b 04 mov ecx,DWORD PTR [ebx+0x4]
748a66: 89 43 08 mov DWORD PTR [ebx+0x8],eax
748a69: c6 04 08 00 mov BYTE PTR [eax+ecx*1],0x0
748a6d: 8b 4c 24 1c mov ecx,DWORD PTR [esp+0x1c]
748a71: 5f pop edi
748a72: 5e pop esi
748a73: 8b c5 mov eax,ebp
748a75: 5d pop ebp
748a76: 5b pop ebx
748a77: 64 89 0d 00 00 00 00 mov DWORD PTR fs:0x0,ecx
748a7e: 83 c4 18 add esp,0x18
748a81: c3 ret 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?
lmichaelis
commented
2 years ago 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 :>
sei-eschwartz
commented
2 years ago I just turned off the relevant sanity checking rule to see if that would be sufficient, but no, we get another error:
Contradictory information about merging classes: Method1=0x7f7ee4 Method2=0x7f7f58
rTTISelfRef(0x85dce0, 0x80d1f0, 0x80d1e0, 0x80d1c0, 0x7f7ee4, '.?AVzCMusicTheme@@').
rTTISelfRef(0x85e010, 0x80d2a0, 0x80d290, 0x80d270, 0x7f7f58, '.?AVzCMusicJingle@@').They are indeed not the same class. So why do we think they should be merged?
reasonMergeClasses_K(0x7f7f58, 0x7f7ee4, 0x4e56f0).
reasonMergeClasses_K(0x7f7ee4, 0x7f7f58, 0x4e5730).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.
reasonClassRelatedMethod_B(function=0x4e4e30, class1=0x4e56f0, class2=0x4e5730, method1=0x4e56f0, method2=0x4e5730).
Can you provide the disassembly for 0x4e4e30 as well?
lmichaelis
commented
2 years ago It's quite a large function. Here's a gist.
sei-eschwartz
commented
2 years ago reasonMergeVFTables_A(constructor, 0x7f7f58, 0x4e5730, 0x7f7f58, 0, factVFTableWrite(0x4e5763, 0x4e5730, 0, 0x7f7f58)).This is connecting CMusicJingle's VFTable with constructor 0x4e5730. But several other methods also install this VFTable:
Concluding factVFTableWrite(0x6176de, 0x617690, 0, 0x7f7f58).
Concluding factVFTableWrite(0x4e578c, 0x4e5780, 0, 0x7f7f58).
Concluding factVFTableWrite(0x4e5763, 0x4e5730, 0, 0x7f7f58).
Concluding factVFTableWrite(0x4e783e, 0x4e76f0, 0, 0x7f7f58).
Concluding factVFTableWrite(0x61774e, 0x617700, 0, 0x7f7f58).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.
sei-eschwartz
commented
2 years ago 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!
lmichaelis
commented
2 years ago 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 :shrug:
sei-eschwartz
commented
2 years ago Going back to the original sanity problem where we were inheriting from two offsets, here is the class hierarchy according to IDA:
.data:0087DA58 ; public class oCViewDocumentMap /* mdisp:0 */ :
.data:0087DA58 ; public class oCViewDocument /* mdisp:0 */ :
.data:0087DA58 ; public class zCViewDialog /* mdisp:0 */ :
.data:0087DA58 ; public class zCViewPrint /* mdisp:0 */ :
.data:0087DA58 ; public class zCViewFX /* mdisp:0 */ :
.data:0087DA58 ; public class zCViewDraw /* mdisp:0 */ :
.data:0087DA58 ; public class zCViewObject /* mdisp:0 */ :
.data:0087DA58 ; public class zCObject /* mdisp:0 */,
.data:0087DA58 ; public class zCViewBase /* mdisp:36 */,
.data:0087DA58 ; public class zCInputCallback /* mdisp:236 */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.
sei-eschwartz
commented
2 years ago I suspect that maybe 0x4e5730 is not a constructor for CMusicJingle
That was wrong. 0x4e5730 is a constructor for CMusicJingle
sei-eschwartz
commented
2 years ago 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.
zCMusicTheme::zCMusicTheme((zCMusicTheme *)the_weird_object);
the_weird_object[0] = (int)&zCMusicTheme::`vftable';
zSTRING::~zSTRING((zSTRING *)probably_part_of_weird_object);
zSTRING::~zSTRING((zSTRING *)&the_weird_object[1]);
zCMusicJingle::zCMusicJingle((zCMusicJingle *)the_weird_object);
zCMusicJingle::~zCMusicJingle((zCMusicJingle *)the_weird_object);
memset(the_weird_object, 0, 0x24u);
the_weird_object[3] = 1;
the_weird_object[6] = 1;
the_weird_object[0] = 36;
the_weird_object[1] = 35;
the_weird_object[2] = 64;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?
sei-eschwartz
commented
2 years ago 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.
sei-eschwartz
commented
2 years ago Here's a better example showing the classes don't even need to be related: https://godbolt.org/z/7T4vb9Yc9
sei-eschwartz
commented
2 years ago 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.
cfcohen
commented
2 years ago ThisPtrAllocation facts...
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 https://github.com/cmu-sei/pharos/issues/211#issuecomment-1038234832, 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: @.***>
sei-eschwartz
commented
2 years ago @sei-ccohen
It's likely that only "type_Heap" is exported as a Prolog fact currently
Seems like a problem :-)
sei-eschwartz
commented
2 years ago @sei-ccohen Just a reminder that you are still looking into why thisPtrAllocation facts are not present for stack objects (I think)
sei-eschwartz
commented
2 years ago @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.
lmichaelis
commented
2 years ago Hm, loading the JSON file into Ghidra using Kaiju produces really weird results. I've picked zSTRING as an example since I know from previous reverse-engineering work that it is a simple wrapper around a std::string, adding some functionality.
Before loading the JSON file, there are basically no classes recognized:
After loading it, classes are shown but zSTRING, has way too many constructors:
Additionally, there seem to be duplicated constructor definitions. According to the analysis 0x0074d8c0 is a constructor zSTRING::zSTRING() but 0x006e7900, 0x006e7a80, 0x006fd960, 0x0070d420 have the same definition though different bodies.
It also seems to find waaay too many members:
sei-eschwartz
commented
2 years ago Thanks, I'll look into this. Definitely sounds like it merged way too many methods into zSTRING.
sei-eschwartz
commented
2 years ago finalClass(0x7f56ec, 0x7f56ec, 0x920, 0x920, 0x599de0, [0x402cd0, 0x41daf0, 0x460a00, 0x46bc00, 0x46c090, 0x46c190, 0x46c290, 0x46c390, 0x46c4e0, 0x46ded0, 0x46e010, 0x4804b0, 0x4804f0, 0x488fe0, 0x489590, 0x4897d0, 0x49cdb0, 0x4d1740, 0x4e4b30, 0x4f7130, 0x517b30, 0x5783f0, 0x593d30, 0x593ee0, 0x599de0, 0x59b690, 0x59b820, 0x59bce0, 0x5d31f0, 0x5d32e0, 0x5d44f0, 0x6a63f0, 0x6e7900, 0x6e7a80, 0x6f6370, 0x6f6580, 0x6fd6a0, 0x6fd960, 0x6fe130, 0x70d420, 0x70d460, 0x70d4b0, 0x717e10, 0x74d770, 0x74d8c0, 0x80b1d8]).
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, 4804B0 ; public: __thiscall oSMenuInfoAttribute::oSMenuInfoAttribute(void). This method installs the same zSTRING vftable at offsets 0 and 14. That alone should probably be enough to tell us zSTRING is being embedded.
Here is the relevant log: Concluding mergeVFTables(0x7f56ec, 0x4804b0). I did not run with lots of debugging, but this is probably coming from the constructor rule of VFTableBelongsToClass. In other words, #214.
sei-eschwartz
commented
2 years ago 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.
sei-eschwartz
commented
2 years ago 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.
cfcohen
commented
2 years ago 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.
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!
sei-eschwartz
commented
2 years ago 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.
sei-eschwartz
commented
2 years ago One such method is 0x49eb80: oCVisFX_Lightning::UpdateBurnVobs
Proposing guessClassRelatedMethod_A(function=0x49eb80, class1=0x488f50, class2=0x488080, method1=0x488f50, method2=0x488db0). Proposing factLateMergeClasses_G1(0x41daf0, 0x48f350, 0x48f350).
0x49eb80 is oCVisFX_Lightning::UpdateBurnVobs 0x488f50 is oCTrajectory::VobCross 0x488db0 is oCTrajectory::SetByList
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:
from idautils import *
from idaapi import *
from idc import *
f = open("/tmp/ground.csv", "w")
for segea in Segments():
for funcea in Functions(segea, get_segm_end(segea)):
functionName = get_func_name(funcea)
f.write ("%s,%#x\n" % (functionName, funcea))from idautils import *
from idaapi import *
from idc import *
f = open("/tmp/ground.csv", "w")
for segea in Segments():
for funcea in Functions(segea, get_segm_end(segea)):
functionName = get_func_name(funcea)
f.write ("%08x\tfunc\t%s\tNone\n" % (funcea, functionName))That will dump in a format our ooanalyzer-symbolizer tool can read.
sei-eschwartz
commented
2 years ago Something like this is working fairly well now: ~/pharos/build/tools/ooanalyzer/tests/ooanalyzer-symbolizer.py ~/ground.csv <(zcat gothic.exe.results.log.gz | fgrep -C5 0x49eb80) | less
I just need to find some time to work backwards to figure out where things start going wrong.
sei-eschwartz
commented
2 years ago So this is odd.
Concluding factVFTableEntry(0x7f56ec, 0x4, 0x80b1d8).0x80b1d8 is a RTTI complete object locator, not a method.
sei-eschwartz
commented
2 years ago Seems to be related to reasonVirtualFunctionCall(0x42c2a5, 0x004701A0 ?Play@zCVideoPlayer@@QAEHH@Z, 0, 0x7f56ec, 0x4).
sei-eschwartz
commented
2 years ago And factVFTableWrite(0x42c1e9, 0x0042C1B0 ?PlayVideo@CGameManager@@QAEHVzSTRING@@H@Z, 0x4, 0x7f56ec).
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.
sei-eschwartz
commented
2 years ago I am looking at this again now. I think the root of the problem is related to the vftable installs. The possibleVFTableWrite fact includes a thisptr. But factVFTableWrite does not. The implication is that any factVFTableWrite should be for the this object. This is not true for factVFTableWrite(0x42c1e9, 0x0042C1B0 ?PlayVideo@CGameManager@@QAEHVzSTRING@@H@Z, 0x4, 0x7f56ec) because it is initializing a zSTRING that is passed as an argument.
sei-eschwartz
commented
2 years ago 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.
sei-eschwartz
commented
2 years ago 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 0x00403470 ?_Tidy@?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AAEX_N@Z for some reason. Ah, this is probably why:
Proposing factLateMergeClasses_G1(0x0041DAF0 ??_EzSTRING@@UAEPAXI@Z, 0x00403470 ?_Tidy@?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AAEX_N@Z, 0x00403470 ?_Tidy@?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AAEX_N@Z).
tryBinarySearch on tryMergeClasses[(0x0041DAF0 ??_EzSTRING@@UAEPAXI@Z, 0x00403470 ?_Tidy@?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AAEX_N@Z)]
Guessing mergeClasses(0x0041DAF0 ??_EzSTRING@@UAEPAXI@Z, 0x00403470 ?_Tidy@?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AAEX_N@Z).
Merging class 0x00403470 ?_Tidy@?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AAEX_N@Z into 0x0041DAF0 ??_EzSTRING@@UAEPAXI@Z ...@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.
lmichaelis
commented
2 years ago The number of methods looks fine now, though they are all marked as constructors now:
Some definitions appear twice, probably because of that (see 0074d8c0 and 00717e10, for example). Curiously, this specific thing only seems to happen to zSTRING. I also see 00443510 and 00443070 as having the same definition but different bodies and zPATH having two destructors (004485f0 and 0048d5a0), just to mention some examples.
I've caused quite a stir, haven't I? I hope it's not causing you too much trouble :)
sei-eschwartz
commented
2 years ago Thanks again. I will look into these. I looked only at the .results file so it was not immediately apparent that all the zSTRING methods were constructors!
I've caused quite a stir, haven't I? I hope it's not causing you too much trouble :)
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.
sei-eschwartz
commented
2 years ago I had a quick look at this. Here is the zSTRING class:
finalClass(0x004013C0 ??0zSTRING@@QAE@PBD@Z, 0, 0x4c, 0x4c, 0, [0x004013C0 ??0zSTRING@@QAE@PBD@Z, 0x00417730 ??0zSTRING@@QAE@ABV0@@Z, 0x00445EC0 ??AzSTRING@@QAEAADI@Z, 0x0046BAE0 ?Lower@zSTRING@@QAEAAV1@XZ, 0x0046BC00 ?Upper@zSTRING@@QAEAAV1@XZ, 0x0046BE70 ?Align@zSTRING@@QAEAAV1@W4zTSTR_ALIGN@@HD@Z, 0x0046C630 ?PickWord@zSTRING@@QBE?AV1@IABV1@0@Z, 0x0046C920 ?PickWordPos@zSTRING@@QBEPBDIABV1@0@Z, 0x0046CAA0 ?PickWord_Old@zSTRING@@QBE?AV1@HABV1@@Z, 0x0046CB80 ?Insert@zSTRING@@QAEHIABV1@@Z, 0x0046CE50 ?Overwrite@zSTRING@@QAEHIABV1@@Z, 0x0046D170 ?DeleteRight@zSTRING@@QAEHI@Z, 0x0046D280 ?Delete@zSTRING@@QAEHIK@Z, 0x0046D390 ?Deleted@zSTRING@@QBE?AV1@IK@Z, 0x0046D4F0 ?Delete@zSTRING@@QAEHABV1@W4zTSTR_KIND@@@Z, 0x0046D6D0 ?Deleted@zSTRING@@QBE?AV1@ABV1@W4zTSTR_KIND@@@Z, 0x0046D980 ?Copied@zSTRING@@QBE?AV1@IK@Z, 0x0046DDD0 ?Copied@zSTRING@@QBE?AV1@ABV1@ABW4zTSTR_KIND@@@Z, 0x0046DED0 ?TrimLeft@zSTRING@@QAEXD@Z, 0x0046E010 ?TrimRight@zSTRING@@QAEXD@Z, 0x0046E1C0 ?Search@zSTRING@@QBEHHPBDI@Z, 0x0046E2D0 ?SearchRev@zSTRING@@QBEHABV1@I@Z, 0x004CFA80 ??4zSTRING@@QAEAAV0@PBD@Z, 0x0058E4B0 ??4zSTRING@@QAEAAV0@ABV0@@Z, 0x0058E610 ?Clear@zSTRING@@QAEXXZ, 0x0058E6E0 ?ToInt@zSTRING@@QBEJXZ, 0x0058E700 ?ToFloat@zSTRING@@QBEMXZ, 0x0065C6C0 ??YzSTRING@@QAEAAV0@PBD@Z, 0x0065C790 ??8@YAHABVzSTRING@@QBD@Z, 0x006FD6A0 ?LoadFontTexture@zCFont@@AAEHABVzSTRING@@@Z, 0x006FE130 ?LoadFontData@zCFont@@QAEHXZ, 0x007084D0 ??0zSTRING@@QAE@PBV0@@Z, 0x00717F60 ?AllocSpace@zCPar_Symbol@@QAEXXZ, 0x00718270 ?GetDataAdr@zCPar_Symbol@@QAEPAXH@Z, 0x00718660 ?SetFlag@zCPar_Symbol@@QAEXH@Z, 0x00718680 ?HasFlag@zCPar_Symbol@@QAEHH@Z, 0x007186A0 ?SetParent@zCPar_Symbol@@QAEXPAV1@@Z, 0x007186B0 ?GetParent@zCPar_Symbol@@QAEPAV1@XZ, 0x007186D0 ?GetNext@zCPar_Symbol@@QAEPAV1@XZ, 0x00718710 ?GetStackPos@zCPar_Symbol@@QAEXAAHH@Z, 0x00718720 ?SetStackPos@zCPar_Symbol@@QAEXHH@Z, 0x00718730 ?SetValue@zCPar_Symbol@@QAEXHH@Z, 0x00718760 ?SetValue@zCPar_Symbol@@QAEXMH@Z, 0x00718790 ?SetValue@zCPar_Symbol@@QAEXAAVzSTRING@@H@Z, 0x007188E0 ?GetValue@zCPar_Symbol@@QAEXAAHH@Z, 0x00718910 ?GetValue@zCPar_Symbol@@QAEXAAMH@Z, 0x00718940 ?GetValue@zCPar_Symbol@@QAEXAAVzSTRING@@H@Z, 0x00718A90 ?SetLineData@zCPar_Symbol@@QAEXHHHH@Z, 0x00718D40 ?GetLineData@zCPar_Symbol@@QAEXAAH000@Z, 0x00718D80 ?SetFileNr@zCPar_Symbol@@QAEXH@Z, 0x00718E50 ?Save@zCPar_Symbol@@QAEXPAVzFILE@@@Z, 0x00719360 ?LoadFull@zCPar_Symbol@@QAEXPAVzFILE@@@Z, 0x00719550 ?Load@zCPar_Symbol@@QAEXPAVzFILE@@@Z, 0x00719720 ?SetOffset@zCPar_Symbol@@QAEXH@Z, 0x00719730 ?GetOffset@zCPar_Symbol@@QAEHXZ, 0x00719760 ??0zCPar_SymbolTable@@QAE@H@Z, 0x007198B0 ??1zCPar_SymbolTable@@QAE@XZ, 0x00719920 ?Clear@zCPar_SymbolTable@@QAEXXZ, 0x00719B00 ?Save@zCPar_SymbolTable@@QAEXPAVzFILE@@@Z, 0x00719C80 ?Load@zCPar_SymbolTable@@QAEXPAVzFILE@@@Z, 0x0071A2C0 ?GetIndex@zCPar_SymbolTable@@QAEHPAVzCPar_Symbol@@@Z, 0x0071A3B0 ?GetIndex@zCPar_SymbolTable@@QAEHABVzSTRING@@@Z, 0x0071A4A0 ?GetIndex@zCPar_SymbolTable@@QAEHABVzSTRING@@H@Z, 0x0071A5B0 ?Search@zCPar_SymbolTable@@AAIHABVzSTRING@@HH@Z, 0x0071A690 ?GetSymbol@zCPar_SymbolTable@@QAEPAVzCPar_Symbol@@ABVzSTRING@@@Z, 0x0071A730 ?GetSymbol@zCPar_SymbolTable@@QAEPAVzCPar_Symbol@@H@Z, 0x0071A750 ?Insert@zCPar_SymbolTable@@QAEHPAVzCPar_Symbol@@@Z, 0x0071A9B0 ?InsertEnd@zCPar_SymbolTable@@QAEXPAVzCPar_Symbol@@@Z, 0x0071ACD0 ?SetSize@zCPar_SymbolTable@@QAEXH@Z, 0x0071ADA0 ?GetNumInList@zCPar_SymbolTable@@QAEHXZ, 0x0071ADB0 ?GetLastSymbol@zCPar_SymbolTable@@QAEPAVzCPar_Symbol@@XZ, 0x0071ADC0 ?GetFirstSymbol@zCPar_SymbolTable@@QAEPAVzCPar_Symbol@@XZ]).There are a bunch of constructors, but some are clearly not, such as 0x46bc00 ?Upper@zSTRING@@QAEAAV1@XZ, which demangles to public: near near class zSTRING & __thiscall zSTRING::Upper(void). So this technically is not a constructor, but it does return a zSTRING.
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 zSTRING, that is true and is not a bug. zSTRING seems to have constructors for many different types, e.g., zSTRING::zSTRING(int), zSTRING::zSTRING(float), etc.
The double destructors in zPATH does not seem like a bug either; one is the deleting destructor and one is the real destructor. But one could certainly argue that maybe calling them both ~zPATH is not ideal.
lmichaelis
commented
2 years ago Oh! Sorry, I didn't realize you had asked a question :/ I did not flag 0x46bc00 as a constructor but there are multiple constructors of zSTRING which take no parameters (see 0046c190 and 0046c290). Also it now does not recognize any methods defined on zSTRING:
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:
on another try I added
--partitioner=roseand--no-semanticsin an effort to make it work:Both died with the error mentioned above. I can send you the
gothic.exe.factsandooprolog.logfiles via e-mail, if required.Thanks :>