fix(labels): demangle the names a PE actually carries - #254
Open
r0ny123 wants to merge 3 commits into
Open
Conversation
Demangled names were passed through a port of Ghidra's CondensedString before being stored, so <Error as core::fmt::Display>::fmt reached the report as <Error_as_core::fmt::Display>::fmt and A, B became A,B. Ghidra needs that because a Ghidra symbol name cannot contain spaces; SMDA has no such constraint, and already stores names with spaces from another source -- 39 of the 399 names recovered from a PDB on a rust-lld x64 image contain them, because MSVC proc records spell them that way. The condensation was applying one spelling rule to names from a symbol table and another to names from a PDB, in the same report. Removing it exposed a printer bug it had been masking: a function pointer's ABI ran into its fn, giving unsafe extern "C"fn(*mut u8). The closing quote was emitted without the trailing space, and comparisons against the reference implementation could not see it because the condensation deleted that space from both sides. One existing test asserted the wrong spelling and is corrected. Measured against rustc-demangle 0.1.28 on 147 real symbols read out of a rust-lld x64 image: 93 of 147 matched byte for byte before, 146 after dropping the condensation, and 147 after the ABI fix. Cross-checked against the 18 mangled symbols in the reference implementation's own test corpus: 8 match, 9 raise and so keep the name the binary gave them, and 1 differs -- a punycode identifier this port renders as a placeholder, which is pre-existing and left alone here.
src/smda/common/labelprovider/rust_demangler/ is vendored third-party code and carries no attribution. The commit that introduced it says so directly -- it vendors the rust_demangler package by Team bi0s, MIT licensed, with robustness behaviour reimplemented from Ghidra, which is Apache-2.0. Neither is recorded: LICENSE covers only this project's own BSD 2-Clause, there is no NOTICE, and the package has no header. The MIT licence asks that its copyright and permission notice travel with copies and substantial portions, and this code is redistributed in the wheel published to PyPI. Add a NOTICE carrying the MIT copyright line and permission text and a pointer to Apache-2.0 for the Ghidra-derived parts, a docstring on the vendored package pointing at it, and a line in the README credits. The in-code comments this attribution was drawn from named "Ghidra's rust-demangle.c", and no such file exists: Ghidra's Rust demanglers are Java, at Ghidra/Features/Rust/src/main/java/ghidra/app/plugin/core/ analysis/rust/demangler/RustDemanglerV0.java and RustDemanglerLegacy.java. Name those instead, in the NOTICE and in the two comments that were the source of the error, and record that Ghidra's own V0 demangler is a port of the rustc-demangle crate, which is the real upstream of the behaviour. The NOTICE lists every vendored component rather than only the one that prompted it, so Tarjan.py and DominatorTree.py -- both already credited in the README paragraph the new line joins -- appear alongside it. No pyproject change is needed: setuptools' default license-file glob already matches NOTICE*, and a build confirms it reaches both the wheel (dist-info/licenses/NOTICE) and the sdist.
Two reasons a PE reached the report with mangled names. RustSymbolProvider recovered nothing at all from a PE. Its COFF loop skipped a symbol when Symbol.section was None, and lief never populates that attribute for PE -- PeSymbolProvider already says so in a comment and resolves through section_idx instead. On the bundled mingw-linked Rust fixture, 0 of 4818 symbols carry a section while 4366 carry a usable section_idx, so the guard rejected every one, the provider contributed nothing, and PeSymbolProvider's raw spellings won by default: 2098 names reached the report spelled _RNv... Resolving through section_idx recovers those 2098 and none of them stay mangled. PeSymbolProvider never demangled anything. ElfSymbolProvider and MachoSymbolProvider both route recovered names through a demangler, while this one stored them verbatim, so a mingw-built C++ PE reported _ZN12FileExplorerC2Ev rather than a signature. It now applies the same Itanium helper ElfSymbolProvider uses, which leaves every non-Itanium name untouched. Making that COFF loop live also made the gate in front of it matter, and it was the wrong one. _is_rust_symbol tested the prefixes alone, but legacy Rust mangling shares _ZN with the C++ Itanium ABI, and this provider is consulted before the format providers -- so on a Rust binary that also carries C++ symbols it would have claimed names like _ZN4test4funcEv and replaced the full Itanium signature "test::func()" with the "test::func" the Rust legacy demangler degrades it to. It now uses the shared evidence gate, which parses a name before claiming it. Recovery on the fixture is unchanged at 2098 names. No bundled PE carried Itanium C++ symbols to show the second fix on, so add one: a small C++ translation unit compiled for x86_64-w64-mingw32 by g++ 16.2.0. Before the change it yields three mangled names and nothing readable; after it, three readable signatures and nothing mangled. The mocks in testRustSymbolProvider modelled a PE symbol through Symbol.section, which no real PE symbol has, so they exercised a path that could not work on a real binary; they now carry section_idx, and the provider reads it directly rather than through a getattr default that would turn a future lief rename back into "skip every symbol". Several tests used _ZN3foo3barE as their Rust fixture, which has no 17h<hash> suffix and so is Itanium C++ rather than legacy Rust; they now use a name that really is Rust-legacy, and the PE provider test expects foo::bar from the C++ demangler.
r0ny123
force-pushed
the
fix/pe-symbol-demangling
branch
from
August 15, 2026 05:22
942356f to
f33431a
Compare
Contributor
Author
|
Rebased onto a linear stack after a review pass found that this branch and #249 both touch The order is now #249 → #250 → #254 → #255, and every pair of the open branches merges cleanly. Two behaviour findings from the same pass are fixed here:
|
This was referenced Aug 15, 2026
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 reasons a PE reaches the report with mangled symbol names, both measured against real binaries.
Part of a linear stack: #249 → #250 → #254 → #255. This branch and #249 both touch
RustSymbolProviderand do not merge cleanly on their own; resolving that conflict in this branch's favour would leave an import of a module #249 deletes, anImportErroronimport smdathat no test on either branch would catch. Stacked, the order resolves it.RustSymbolProvider recovered nothing at all from a PE
Its COFF loop skipped a symbol when
symbol.section is None. lief never populates that attribute for PE — which this repo already knows, becausePeSymbolProvidercarries a comment saying so and resolves throughsection_idxinstead, fixed under #229. The identical sibling here was not swept at the time.Measured on the bundled
rust_pe_gnu_xoredfixture (a mingw-linked Rust build):So the guard rejected every symbol, the provider contributed zero names, and
PeSymbolProvider's raw spellings won by default. The report carried 2098 names spelled_RNv…. With the guard resolving throughsection_idx, the provider recovers 2098 names and none of them are mangled.PeSymbolProvider never demangled anything
ElfSymbolProviderandMachoSymbolProviderboth route recovered names through a demangler; the PE provider stored them verbatim, so a mingw-built C++ PE reported_ZN12FileExplorerC2Evrather than a signature. It now applies the samedemangle_itanium_symbolhelperElfSymbolProvideruses, which leaves non-Itanium names — including Rust ones, which belong toRustSymbolProvider— untouched.There was no bundled PE carrying Itanium C++ symbols to show this on: the Rust fixture's names are all Rust-mangled and the other PEs have no symbol table.
tests/cxx_pe_gnu_xoredis a small C++ translation unit compiled forx86_64-w64-mingw32by g++ 16.2.0, added so the path has a real binary behind it. Before and after, on that file:giving
demo::Widget::Widget(),demo::Widget::~Widget()and the fullmeasure(std::__cxx11::basic_string<…> const&, double) constsignature.The gate in front of the revived loop
Bringing that COFF loop to life also made the test in front of it matter, and it was the wrong one.
_is_rust_symbolchecked the prefixes alone, but legacy Rust mangling shares_ZNwith the C++ Itanium ABI, andRustSymbolProvideris consulted before the format providers. On a Rust binary that also carries C++ symbols it would therefore have claimed names like_ZN4test4funcEvand replaced the correcttest::func()with thetest::funcits legacy demangler degrades that name to — shadowing the very demangling this PR adds. It now usesis_rust_language_evidence, which parses a name before claiming it. Recovery on the fixture is unchanged at 2098 names, so the stricter gate costs nothing here.Scope of the sweep
The first bug's class is a PE symbol resolved through
Symbol.sectionrather thansection_idx. After this change no such site remains in the tree — the only surviving mention of.sectionin the label providers is the comment explaining why not to use it.The second bug's class is a provider that stores a recovered name without routing it through the demangler for its format. ELF, Mach-O and now PE all demangle; the Go, Delphi and CIL providers handle name spaces that are not mangled in this sense.
Validation
Real binaries only, no synthetic symbol names:
rust_pe_gnu_xored(mingw Rust)cxx_pe_gnu_xored(mingw C++, new)komplex_xored(Mach-O C++)Still open after this
MSVC decoration (
?name@@YAXXZ) is not demangled by this PR; #255, stacked on top, adds that.