Skip to content

fix(core): keep an import named like an ordinal from becoming one - #261

Open
r0ny123 wants to merge 1 commit into
danielplohmann:masterfrom
r0ny123:fix/pe-synthesis-ordinal-collision
Open

fix(core): keep an import named like an ordinal from becoming one#261
r0ny123 wants to merge 1 commit into
danielplohmann:masterfrom
r0ny123:fix/pe-synthesis-ordinal-collision

Conversation

@r0ny123

@r0ny123 r0ny123 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

A PE imports either by name or by ordinal — that is a bit in the thunk, not a property of the string. The report keeps only the string, so "#5" is ambiguous by the time synthesis reads it back: it is what the import parsers write for an ordinal nothing resolves, and it is also a legal PE import name.

PeSynthesizer._parseOrdinal read every #N as an ordinal. An import really named that way was therefore written as an ordinal thunk, and came back from the synthesized binary under a different name entirely.

This is the fourth and last hit from the sweep behind #260, which I had triaged there as needing a report-schema change. It does not — the information to settle it is already on both sides.

What settles it

An ordinal is a WORD. A number above 0xFFFF never came from an import table, so #99999 can only be a name. Writing it as an ordinal did not merely mislabel it, it truncated the value into a different import.

The parsers only write #N when no table resolves it. OrdinalHelper.resolveOrdinal("ws2_32.dll", 1) returns accept, so the parsers would never have written #1 for that DLL — a report holding it must mean a real import spelled that way. The synthesizer now applies the parsers' own test as its inverse.

What remains ambiguous is a real import named #N where that ordinal resolves nowhere for its DLL. That one still synthesizes as an ordinal — and re-analysing the result yields #N again, so the report round-trips unchanged. The divergence is invisible in the report's own terms, which is the property worth having here.

One structural change

The hint table and the thunk writer each decided independently which entries were ordinals, and each derived the DLL key separately (dll_name in one, dll or "unknown.dll" in the other). Now that the decision depends on the DLL, disagreeing would mean a KeyError on the hint lookup rather than a wrong byte. Both ends key on the same value, so they cannot drift.

Validation

The new test fails on the pre-fix tree and passes after it. Two existing robustness tests stubbed _parseOrdinal with a one-argument lambda and are updated — worth noting as the reason a private-method signature is not as private as it looks.

check result
full test suite 1182 passed, 1 skipped, 1480 subtests
lint and format check clean
type check exit 0, no new diagnostics
diff coverage against master 100% (12/12 changed lines)

A PE imports by name or by ordinal, and the report keeps only the string,
so "#5" in a report is ambiguous: it is what the import parsers write for
an ordinal nothing resolves, and it is also a legal import name. The
synthesizer read every "#N" as an ordinal, so an import really named that
way was written as an ordinal thunk and came back under a different name.

Two facts settle it without the report having to carry more. An ordinal
is a WORD, so a larger number never came from an import table at all -
and writing it as one truncated it into a different import. And the
parsers write "#N" only for an ordinal no table resolves, so a name that
does resolve cannot have come from them. On ws2_32.dll, "#1" is a real
name, because an ordinal 1 there would have been written as accept.

Both ends now key the decision on the same DLL name rather than deriving
it twice, so the hint table and the thunks cannot disagree about which
entries are ordinals.
@r0ny123
r0ny123 force-pushed the fix/pe-synthesis-ordinal-collision branch from 947a469 to b0c3d19 Compare August 18, 2026 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant