fix(dalvik): decide from what an object is, not from text about it - #260
Open
r0ny123 wants to merge 1 commit into
Open
fix(dalvik): decide from what an object is, not from text about it#260r0ny123 wants to merge 1 commit into
r0ny123 wants to merge 1 commit into
Conversation
Two places asked a rendered or composed string what it was, and the string could answer wrongly. A type object that exposes neither a full name nor a name falls back to its repr. That repr was normalized first and the result then tested for the "<lief." prefix - but normalization is what rewrites dots to slashes and wraps the result as "L...;", so it strips the very prefix the test looks for. "<lief.DEX.Type object at 0x7f00>" became "L<lief/DEX/Type object at 0x7f00>;", which passes the guard and reaches a report field as a plausible type descriptor carrying a heap address. The raw string is now tested before normalization, as the fallback ten lines below already does. A method discovered in a gap carries a name this module writes, "orphan_code_item@0x...", and the label was chosen by matching that prefix. A DEX method name is arbitrary UTF-8, so a real method may carry it too, and one that did would keep the raw name instead of the class-and-prototype spelling the resolver builds. The label now follows from the object's type, which is what the producer already knows. Both tests fail before the change and pass after it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two places in the Dalvik backend asked a string what it was, where the string could answer wrongly. Both were found by sweeping the tree for the class after fixing an instance of it elsewhere (#255), and neither is reachable from the demangling work that surfaced them — they are pre-existing and independent, so they are here rather than folded into that stack.
A type object's repr can reach a report field dressed as a type descriptor
DexReferenceResolver._formatTypefalls back tostr(value)when a type object exposes neitherfullnamenorname. That fallback normalized the string first and then tested the result for a<lief.prefix, to reject an object repr. But normalization is exactly what rewrites dots to slashes and wraps the result asL...;, so it strips the prefix the test looks for:<lief.DEX.Type object at 0x7f0000000000>L<lief/DEX/Type object at 0x7f0000000000>;startswith("<lief.")So the guard never fires, and a raw object repr — carrying a heap address — reaches a report's type field looking like a legitimate descriptor. The fallback ten lines below in the same method already gets this right, testing the raw string before normalizing and carrying a comment explaining why. This one now does the same.
A real method may be named like a synthetic one
A code item discovered in a gap gets a synthetic method object whose name this module writes as
orphan_code_item@0x.... The label was then chosen by matching that prefix back off the name. A DEX method name is arbitrary UTF-8 with no format restriction, so a real method can carry the same string, and one that did would keep the raw name instead of the class-and-prototype spellingformatMethodbuilds. The producer already knows the fact — the object's type — so the label follows fromisinstancerather than from the text.The class, since it is worth naming
A semantic decision made by inspecting text this codebase itself produced, instead of being told the fact by whatever produced it. The sweep separated these from the many
startswithtests that inspect genuinely external input — capstone mnemonics, raw mangled names, PDB string-table fields — which are not in the class.A fourth hit —
PeSynthesizer._parseOrdinalreading back the"#N"the import parsers write for an unresolvable ordinal — was triaged here as needing a report-schema change, and so left alone. That was wrong, and it is fixed in #261: the information to settle it already exists on both sides, since an ordinal is aWORDand the parsers only write"#N"when no table resolves it. No schema change was needed.Also checked and found sound:
_formatTypecaches byid(), which is only safe if every type object reaching it outlives the cache. lief interns its wrappers (m.cls is m.clsholds, and 200 transient accesses across the bundled fixture produce 2 distinct ids), so the existing comment's premise is correct. It does bite synthetic objects in tests, which is why the new test holds its two fixtures for its whole body — worth knowing before writing another one.Validation
Both new tests fail on the pre-fix tree and pass after it.