Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DKII consistency failures #232

Closed
sei-eschwartz opened this issue Jul 11, 2022 · 5 comments
Closed

DKII consistency failures #232

sei-eschwartz opened this issue Jul 11, 2022 · 5 comments

Comments

@sei-eschwartz
Copy link
Collaborator

Not sure if this is still related to the original problem but it still doesn't run through:

Contradictory information about constructor: factConstructor(0x5de050) but reasonNOTConstructor(0x5de050)
Constraint checks failed, retracting guess!
failed.
Consistency checks failed.
Contradictory information about constructor: factConstructor(0x600400) but reasonNOTConstructor(0x600400)
Constraint checks failed, retracting guess!
tryBinarySearch completely failed on [0x5de050] and will now backtrack to fix an upstream problem.
Refusing to backtrack into reasoningLoop to fix an upstream problem because backtrackForUpstream/0 is not set.
This likely indicates that there is a problem with the OO rules.
Please report this failure to the Pharos developers!
 [150] prolog_stack:get_prolog_backtrace(100,[frame(150,clause(<clause>(0x5ce281eb4d00),6),_5626440)|_5626428],[goal_term_depth(100)]) at /usr/local/lib/swipl/library/prolog_stack.pl:137
 [149] throw_with_backtrace(error(system_error(upstreamProblem))) at /usr/local/share/pharos/prolog/oorules/util.pl:185
  [26] solve_internal at /usr/local/share/pharos/prolog/oorules/setup.pl:681
  [25] catch(user:solve_internal,_5626664,user:((_5626732=error(resource_error(private_table_space),_5626746)->complain_table_space(ooscript);_5626796=error(resource_error(stack),_5626810)->complain_stack_size(ooscript);true),throw(_5626842))) at /usr/local/lib/swipl/boot/init.pl:562
  [24] solve(ooscript) at /usr/local/share/pharos/prolog/oorules/setup.pl:617

Originally posted by @Trass3r in #209 (comment)

@sei-eschwartz
Copy link
Collaborator Author

I might see what is going wrong...

reasonMergeVFTables_A(constructor, 0x672900, 0x6728f8, 0x672900, 0, factVFTableWrite(0x5dd7cf, 0x5dd760, 0, 0x672900)).
Concluding mergeVFTables(0x672900, 0x6728f8).

So we decide that this vftable belongs to the current constructor based on the vftable write. Here is the vftable write:

.text:005DD7C0 call ??2@YAPAXI@Z ; operator new(uint)
.text:005DD7C5 add esp, 4
.text:005DD7C8 cmp eax, edi
.text:005DD7CA jz short loc_5DD7DB
.text:005DD7CC mov [eax+8], edi
.text:005DD7CF mov dword ptr [eax], offset some_vftable_0

In other words, we install this vftable into a new heap-allocated object, not the "this" object this constructor is constructing.

I think this is a side-effect of a change we made recently that exported possibleVFTableFacts for non-this objects. The solution is to validate that the vftable write is in "this" object. I am trying this now.

sei-eschwartz added a commit to sei-eschwartz/pharos that referenced this issue Jul 11, 2022
sei-eschwartz added a commit to sei-eschwartz/pharos that referenced this issue Jul 11, 2022
@sei-eschwartz
Copy link
Collaborator Author

I think this should fix it: cf77c93

It certainly doesn't fail as quickly, but my run of DKII hasn't completed yet.

@sei-eschwartz
Copy link
Collaborator Author

This causes a regression in our unit tests:

117: FAILED: ooex_vs2010/Lite/oo ooanalyzer results +finalClass(0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void), 0, 0xc, 0xc, 0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void
), [0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void), 0x00402FCC __thiscall std::basic_char_ostream::basic_char_ostream(class std::basic_char_streambuf *, bool)]).+
117: FAILED: ooex_vs2010/Lite/oo ooanalyzer results +finalClass(0x0041238C const std::basic_char_ostream::`vftable', 0x0041238C const std::basic_char_ostream::`vftable', 0, 0, 0, [0x00402766 virtual void * __thiscall std::basic_
char_ostream::`vector deleting destructor'(unsigned int)]).+
117: FAILED: ooex_vs2010/Lite/oo ooanalyzer results -finalClass(0x0041238C const std::basic_char_ostream::`vftable', 0x0041238C const std::basic_char_ostream::`vftable', 0xc, 0xc, 0x0040247E void __thiscall std::basic_char_ostre
am::`vbase destructor(void), [0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void), 0x00402766 virtual void * __thiscall std::basic_char_ostream::`vector deleting destructor'(unsigned int), 0x00402FCC __th
iscall std::basic_char_ostream::basic_char_ostream(class std::basic_char_streambuf *, bool)]).-

We are no longer able to associate the vbase destructor with the correct class.

Binary ninja says the function simplifies to:

sub_40247e:    
0 @ 00402487  *(*(*arg1 + 4) + arg1 + 8 - 8) = 0x41238c    
1 @ 00402490  *(arg1 + 8) = 0x412384    
2 @ 0040249c  return sub_403d09(arg1 + 8)

Statement 0 installs std::ostream::vftable. This is obviously accessing a virtual base offset, which is why the change breaks things. This is unavoidable I think until we improve our support for virtual bases.

@sei-eschwartz
Copy link
Collaborator Author

@Trass3r Take a look at this?
DKII.EXE.json.gz

@Trass3r
Copy link
Contributor

Trass3r commented Jan 9, 2023

The patch in master...sei-eschwartz:pharos:master (also mentioned in #227 (comment)) indeed made it run to the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants