From aa2b5dae07bb7c9bb0a8f8db96f95ffd6504703d Mon Sep 17 00:00:00 2001 From: Weitao Sun Date: Mon, 29 Jun 2026 15:00:29 -0400 Subject: [PATCH 1/4] Fix multiple PE/COFF parsing issues (#9169, #9170, #9171) Bug #9169 - COFF symbol table entries (peview.cpp + coffview.cpp): - Cap section name string table reads at 1024 bytes per name - Validate section- and symbol-name string table offsets against the declared string table size before reading, and use 64-bit arithmetic (and the actual per-record size in coffview.cpp) when computing string table offsets - Add a configurable limit (default 1M) on how many COFF symbol table entries receive full name resolution, typing, and aux record definitions; every table slot still gets a data variable and marker symbol in coffview.cpp so relocations can resolve any symbol index the file declares - Add a per-name length cap (default 32 KB) via ReadCString(maxSymNameLen) - Add a total budget (default 1 GB) on accumulated symbol name bytes, weighted for the additional copies a resolved name ends up retained in once a symbol is created for it; cache names by string table offset so repeated offsets are read and counted once - Expose all limits as user-configurable loader settings (loader.pe.* and loader.coff.*), each accepting 0 to disable the corresponding check Bug #9170 - PointerToRawData not 0x200-aligned (peview.cpp): - For PE32/PE32+, always round PointerToRawData DOWN to the nearest 0x200 boundary, matching the Windows loader's hardcoded behaviour regardless of the FileAlignment field value Bug #9171 - SizeOfRawData not rounded to FileAlignment (peview.cpp): - For PE32/PE32+, round SizeOfRawData UP to the next FileAlignment multiple so that bytes between the declared raw size and the alignment boundary are included in the binary view, matching what Windows maps; cap the result at the remaining bytes in the file, clamping before narrowing to the 32-bit section field - Use uint64_t arithmetic for the rounding to prevent overflow when sizeOfRawData is near UINT32_MAX Co-Authored-By: Claude Sonnet 4.6 --- view/pe/coffview.cpp | 182 ++++++++++++++++++++++++++++++++----------- view/pe/peview.cpp | 140 +++++++++++++++++++++++++++++---- view/pe/peview.h | 9 +++ 3 files changed, 273 insertions(+), 58 deletions(-) diff --git a/view/pe/coffview.cpp b/view/pe/coffview.cpp index 7c77c5d73..6f88ff481 100644 --- a/view/pe/coffview.cpp +++ b/view/pe/coffview.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include "coffview.h" @@ -258,21 +259,25 @@ bool COFFView::Init() if (errno == 0 && offset > 0) { BinaryReader stringReader(GetParentView(), LittleEndian); - uint64_t stringTableBase = header.coffSymbolTable + (header.coffSymbolCount * 18); + // Compute the string table offset using 64-bit arithmetic and the + // actual per-symbol record size (18 bytes normally, 20 for BigCOFF). + uint64_t stringTableBase = header.coffSymbolTable + ((uint64_t)header.coffSymbolCount * sizeofCOFFSymbol); stringReader.Seek(stringTableBase); uint32_t stringTableLen = stringReader.Read32(); if ((stringTableBase + stringTableLen) > GetParentView()->GetEnd()) { m_logger->LogError("Cannot resolve section name \"%s\": String table is invalid length", name); } - else if (stringTableBase + offset < GetParentView()->GetEnd()) + else if (offset < stringTableLen) { sectionNameReader.Seek(stringTableBase + offset); - resolvedName = sectionNameReader.ReadCString(); + // Section names longer than 1024 bytes are not meaningful; cap the read + // to bound the allocation. + resolvedName = sectionNameReader.ReadCString(1024); } else { - m_logger->LogError("Cannot resolve section name \"%s\": Offset is past end of string table", name); + m_logger->LogError("Cannot resolve section name \"%s\": Offset %u exceeds the string table size %u", name, offset, stringTableLen); } } } @@ -866,6 +871,29 @@ bool COFFView::Init() // TODO: combine the aux symbol record struct types into a union: // StructureBuilder coffAuxSymbolRecordBuilder(UnionStructureType); + // A limit of 0 disables the corresponding check. + uint64_t maxSymCount = PE_DEFAULT_MAX_COFF_SYMBOL_COUNT; + uint64_t maxSymNameLen = PE_DEFAULT_MAX_COFF_SYMBOL_NAME_LENGTH; + uint64_t maxTotalSymNameBytes = PE_DEFAULT_MAX_TOTAL_COFF_SYMBOL_NAME_MB * 1024 * 1024; + if (settings && settings->Contains("loader.coff.maxCoffSymbolCount")) + maxSymCount = settings->Get("loader.coff.maxCoffSymbolCount", this); + if (settings && settings->Contains("loader.coff.maxCoffSymbolNameLength")) + maxSymNameLen = settings->Get("loader.coff.maxCoffSymbolNameLength", this); + if (settings && settings->Contains("loader.coff.maxTotalCoffSymbolNameBytes")) + maxTotalSymNameBytes = settings->Get("loader.coff.maxTotalCoffSymbolNameBytes", this) + * 1024 * 1024; + // A name length limit of 0 means no limit; ReadCString takes an actual byte count, + // so map it to the largest representable value instead of reading zero bytes. + if (!maxSymNameLen) + maxSymNameLen = UINT64_MAX; + + // Every symbol table slot gets a data variable and a marker symbol below, so that + // relocations can resolve any symbol index the file declares. maxCoffSymbolCount only + // bounds how many of those slots also get full name resolution, typing, and aux + // record definitions, which are the more expensive per-symbol steps. + uint64_t symbolAnnotationLimit = + maxSymCount ? std::min(header.coffSymbolCount, maxSymCount) : header.coffSymbolCount; + size_t symbolTableSize = header.coffSymbolCount * sizeofCOFFSymbol; auto lastSection = m_sections.back(); symbolTableAdjustedOffset = header.coffSymbolTable - lastSection.pointerToRawData + lastSection.virtualAddress; @@ -887,11 +915,11 @@ bool COFFView::Init() DefineAutoSymbol(new Symbol(DataSymbol, "__symtab", coffSymbolTableBase, NoBinding)); BinaryReader stringReader(GetParentView(), LittleEndian); - uint64_t stringTableBaseRaw = header.coffSymbolTable + ((uint64_t) header.coffSymbolCount * sizeofCOFFSymbol); + uint64_t stringTableBaseRaw = header.coffSymbolTable + ((uint64_t)header.coffSymbolCount * sizeofCOFFSymbol); stringReader.Seek(stringTableBaseRaw); - uint32_t stringTableSize = stringReader.Read32(); - if ((stringTableBaseRaw + stringTableSize) > GetParentView()->GetEnd()) + uint32_t stringTableSize; + if (!stringReader.TryRead32(stringTableSize) || (stringTableBaseRaw + stringTableSize) > GetParentView()->GetEnd()) { throw COFFFormatException("invalid COFF string table size"); } @@ -914,8 +942,18 @@ bool COFFView::Init() DefineAutoSymbol(new Symbol(DataSymbol, "__strtab", m_imageBase + stringTableBase + 4, NoBinding)); + // Symbol names are looked up by string table offset before being read, so entries + // that share an offset only pay for one read and count once toward the name budget. + std::unordered_map symbolNameCache; + uint64_t totalSymNameBytesRead = 0; + bool nameBudgetExceeded = false; for (size_t i = 0; i < header.coffSymbolCount; i++) { + // Every slot still gets the marker symbol below regardless of this limit, so + // relocations can resolve symbols past it; only the richer per-symbol work + // (name resolution, typing, aux records) is bounded. + bool annotate = ((uint64_t)i < symbolAnnotationLimit) && !nameBudgetExceeded; + reader.Seek(header.coffSymbolTable + (i * sizeofCOFFSymbol)); uint32_t e_zeroes = reader.Read32(); uint32_t e_offset = reader.Read32(); @@ -946,10 +984,29 @@ bool COFFView::Init() symbolName = stringReader.ReadCString(8); symbolName = symbolName.substr(0, strlen(symbolName.c_str())); } - else + else if (annotate && e_offset < stringTableSize) { - stringReader.Seek(stringTableBaseRaw + e_offset); - symbolName = stringReader.ReadCString(); + auto cached = symbolNameCache.find(e_offset); + if (cached != symbolNameCache.end()) + { + symbolName = cached->second; + } + else if (!maxTotalSymNameBytes || totalSymNameBytesRead < maxTotalSymNameBytes) + { + stringReader.Seek(stringTableBaseRaw + e_offset); + symbolName = stringReader.ReadCString(maxSymNameLen); + // Each name ends up retained in more than one copy once a symbol is + // created for it, so weight the budget accordingly rather than counting + // only the bytes read here. + totalSymNameBytesRead += (uint64_t)symbolName.size() * 4; + symbolNameCache.emplace(e_offset, symbolName); + } + else + { + m_logger->LogWarn("Total COFF symbol name bytes exceeded limit %" PRIu64 + ", limiting further symbol name resolution.", maxTotalSymNameBytes); + nameBudgetExceeded = true; + } } BNSymbolBinding binding; @@ -972,54 +1029,60 @@ bool COFFView::Init() } uint8_t baseType = (e_type >> 4) & 0x3; - switch (baseType) + if (annotate) { - case IMAGE_SYM_DTYPE_NULL: // no derived type + switch (baseType) { - if (virtualAddress) - AddCOFFSymbol(DataSymbol, "", symbolName, virtualAddress, binding); - break; - } - case IMAGE_SYM_DTYPE_POINTER: // pointer to base type - { - break; - } - case IMAGE_SYM_DTYPE_FUNCTION: // function that returns base type - { - if (virtualAddress) + case IMAGE_SYM_DTYPE_NULL: // no derived type + { + if (virtualAddress) + AddCOFFSymbol(DataSymbol, "", symbolName, virtualAddress, binding); + break; + } + case IMAGE_SYM_DTYPE_POINTER: // pointer to base type + { + break; + } + case IMAGE_SYM_DTYPE_FUNCTION: // function that returns base type { - if (!isCLRBinary) + if (virtualAddress) { - auto functionAddress = virtualAddress; - if (header.machine == IMAGE_FILE_MACHINE_ARMNT) + if (!isCLRBinary) { - // NOTE: for IMAGE_FILE_MACHINE_ARMNT, there are only thumb2 functions, - // so we force the low bit on for all function symbols - functionAddress |= 1; + auto functionAddress = virtualAddress; + if (header.machine == IMAGE_FILE_MACHINE_ARMNT) + { + // NOTE: for IMAGE_FILE_MACHINE_ARMNT, there are only thumb2 functions, + // so we force the low bit on for all function symbols + functionAddress |= 1; + } + AddCOFFSymbol(FunctionSymbol, "", symbolName, functionAddress, binding); + } + else if (!clrFunction) + { + AddCOFFSymbol(DataSymbol, "", symbolName, virtualAddress, binding); } - AddCOFFSymbol(FunctionSymbol, "", symbolName, functionAddress, binding); - } - else if (!clrFunction) - { - AddCOFFSymbol(DataSymbol, "", symbolName, virtualAddress, binding); } + break; } - break; - } - case IMAGE_SYM_DTYPE_ARRAY: // array of base type - { - break; + case IMAGE_SYM_DTYPE_ARRAY: // array of base type + { + break; + } + default: + break; } - default: - break; } + // Define a data variable and marker symbol for every raw table slot, + // independent of the annotation limit, so relocations can look up any + // symbol index the file declares. auto symbolVirtualAddress = symbolTableAdjustedOffset + (i * sizeofCOFFSymbol); DefineDataVariable(m_imageBase + symbolVirtualAddress, Type::NamedType(this, coffSymbolTypeName)); string symbolStructName = "__symbol(" + symbolName + ")"; DefineAutoSymbol(new Symbol(DataSymbol, symbolStructName, m_imageBase + symbolVirtualAddress, NoBinding)); - if (e_zeroes == 0) + if (annotate && e_zeroes == 0 && e_offset < stringTableSize) { DefineDataVariable(m_imageBase + stringTableBase + e_offset, Type::ArrayType(Type::IntegerType(1, true, "char"), symbolName.length() + 1)); string symbolStringName = "__symbol_name(" + symbolName + ")"; @@ -1027,13 +1090,13 @@ bool COFFView::Init() DEBUG_COFF(AddDataReference(m_imageBase + symbolVirtualAddress, m_imageBase + stringTableBase + e_offset)); } - if (e_sclass == IMAGE_SYM_CLASS_STATIC && e_value == 0) + if (annotate && e_sclass == IMAGE_SYM_CLASS_STATIC && e_value == 0) { size_t sectionHeaderOffset = sectionHeadersOffset + (e_scnum - 1) * sizeof(COFFSectionHeader); (void)sectionHeaderOffset; DEBUG_COFF(AddDataReference(m_imageBase + symbolVirtualAddress, m_imageBase + sectionHeaderOffset)); } - else if (e_sclass == IMAGE_SYM_CLASS_EXTERNAL && e_value == 0 && e_scnum == IMAGE_SYM_UNDEFINED) + else if (annotate && e_sclass == IMAGE_SYM_CLASS_EXTERNAL && e_value == 0 && e_scnum == IMAGE_SYM_UNDEFINED) { if (baseType == IMAGE_SYM_DTYPE_FUNCTION) { @@ -1046,7 +1109,7 @@ bool COFFView::Init() } // Reify auxiliary symbol record entries - for (size_t j = 0; j < e_numaux; j++) + for (size_t j = 0; annotate && j < e_numaux; j++) { auto auxSymbolAddress = symbolVirtualAddress + ((1 + j) * sizeofCOFFSymbol); if (e_sclass == IMAGE_SYM_CLASS_EXTERNAL && baseType == IMAGE_SYM_DTYPE_FUNCTION && e_scnum > 0) @@ -1709,6 +1772,35 @@ Ref COFFViewType::GetLoadSettingsForData(BinaryView* data) // "description" : "Add function starts sourced from the Structured Exception Handling (SEH) table to the core for analysis." // })"); + settings->RegisterSetting("loader.coff.maxCoffSymbolCount", + R"({ + "title" : "Maximum COFF Symbol Count", + "type" : "number", + "default" : 1000000, + "minValue" : 0, + "maxValue" : 100000000, + "description" : "Maximum number of COFF symbol table entries to fully annotate with names and types. Set to 0 to disable this limit." + })"); + + settings->RegisterSetting("loader.coff.maxCoffSymbolNameLength", + R"({ + "title" : "Maximum COFF Symbol Name Length", + "type" : "number", + "default" : 32768, + "minValue" : 0, + "maxValue" : 1000000, + "description" : "Maximum number of bytes read for a single COFF symbol name from the string table. 32768 comfortably covers the longest real-world Rust mangled names. Set to 0 to disable this limit." + })"); + + settings->RegisterSetting("loader.coff.maxTotalCoffSymbolNameBytes", + R"json({ + "title" : "Maximum COFF Total Symbol Name Budget (MB)", + "type" : "number", + "default" : 1024, + "minValue" : 0, + "maxValue" : 10240, + "description" : "Maximum total memory (in MB) budgeted for all COFF symbol names combined. Set to 0 to disable this limit." + })json"); return settings; } diff --git a/view/pe/peview.cpp b/view/pe/peview.cpp index 452a054f9..dc9eda25d 100644 --- a/view/pe/peview.cpp +++ b/view/pe/peview.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include "peview.h" #include "coffview.h" @@ -803,7 +804,8 @@ bool PEView::Init() if (errno == 0 && offset > 0) { BinaryReader stringReader(GetParentView(), LittleEndian); - uint64_t stringTableBase = header.coffSymbolTable + (header.coffSymbolCount * 18); + // Compute the string table offset using 64-bit arithmetic. + uint64_t stringTableBase = header.coffSymbolTable + ((uint64_t)header.coffSymbolCount * 18); stringReader.Seek(stringTableBase); uint32_t stringTableLen; if (!stringReader.TryRead32(stringTableLen)) @@ -814,14 +816,16 @@ bool PEView::Init() { m_logger->LogError("Cannot resolve section name \"%s\": String table is invalid length", name); } - else if (stringTableBase + offset < GetParentView()->GetEnd()) + else if (offset < stringTableLen) { sectionNameReader.Seek(stringTableBase + offset); - resolvedName = sectionNameReader.ReadCString(); + // Section names longer than 1024 bytes are not meaningful; cap the read + // to bound the allocation. + resolvedName = sectionNameReader.ReadCString(1024); } else { - m_logger->LogError("Cannot resolve section name \"%s\": Offset is past end of string table", name); + m_logger->LogError("Cannot resolve section name \"%s\": Offset %u exceeds the string table size %u", name, offset, stringTableLen); } } } @@ -833,11 +837,40 @@ bool PEView::Init() section.virtualAddress = reader.Read32(); section.sizeOfRawData = reader.Read32(); section.pointerToRawData = reader.Read32(); - if (fileAlignmentValid && (section.pointerToRawData & (resolvedFileAlignment - 1))) + // Windows always rounds PointerToRawData down to a 0x200 boundary for PE32/PE32+, + // regardless of the FileAlignment field value. Apply the same behavior here so that + // our view matches what the Windows loader actually maps into memory. + if ((opt.magic == 0x10b || opt.magic == 0x20b) && (section.pointerToRawData & (PE_SECTION_RAW_DATA_ALIGNMENT - 1))) { - m_logger->LogWarn("PE section[%u] violates file alignment: pointerToRawData: 0x%x. Aligning to 0x%x.", i, - section.pointerToRawData, resolvedFileAlignment); - section.pointerToRawData &= ~(resolvedFileAlignment - 1); + m_logger->LogWarn("PE section[%u]: pointerToRawData 0x%x is not 0x200-aligned, " + "rounding down to 0x%x per Windows loader behavior.", + i, section.pointerToRawData, section.pointerToRawData & ~(PE_SECTION_RAW_DATA_ALIGNMENT - 1)); + section.pointerToRawData &= ~(PE_SECTION_RAW_DATA_ALIGNMENT - 1); + } + // Windows rounds SizeOfRawData up to the nearest FileAlignment multiple for PE32/PE32+. + // Without this, bytes between the raw value and the rounded value are invisible to + // analysis even though the Windows loader maps them. + // Cap at the remaining file bytes to avoid mapping data past the end of the file. + if ((opt.magic == 0x10b || opt.magic == 0x20b) + && section.sizeOfRawData + && (section.sizeOfRawData % resolvedFileAlignment)) + { + // Use uint64_t to avoid overflow when sizeOfRawData is near UINT32_MAX. + uint64_t aligned = ((uint64_t)section.sizeOfRawData + resolvedFileAlignment - 1) + & ~(uint64_t)(resolvedFileAlignment - 1); + uint64_t fileEnd = GetParentView()->GetEnd(); + uint64_t remaining = (fileEnd > section.pointerToRawData) + ? (fileEnd - section.pointerToRawData) : 0; + // Clamp before narrowing to uint32_t: aligned or remaining can exceed UINT32_MAX + // even though sizeOfRawData itself is a 32-bit field. + uint64_t clampedSize = std::min(aligned, remaining); + if (clampedSize > UINT32_MAX) + clampedSize = UINT32_MAX; + uint32_t newSize = (uint32_t)clampedSize; + m_logger->LogWarn("PE section[%u]: sizeOfRawData 0x%x is not FileAlignment " + "(0x%x) aligned, rounding up to 0x%x per Windows loader behavior.", + i, section.sizeOfRawData, resolvedFileAlignment, newSize); + section.sizeOfRawData = newSize; } section.pointerToRelocs = reader.Read32(); section.pointerToLineNumbers = reader.Read32(); @@ -1353,14 +1386,46 @@ bool PEView::Init() // Process COFF symbol table if (header.coffSymbolCount) { + uint64_t maxSymCount = PE_DEFAULT_MAX_COFF_SYMBOL_COUNT; + uint64_t maxSymNameLen = PE_DEFAULT_MAX_COFF_SYMBOL_NAME_LENGTH; + uint64_t maxTotalSymNameBytes = PE_DEFAULT_MAX_TOTAL_COFF_SYMBOL_NAME_MB * 1024 * 1024; + if (settings && settings->Contains("loader.pe.maxCoffSymbolCount")) + maxSymCount = settings->Get("loader.pe.maxCoffSymbolCount", this); + if (settings && settings->Contains("loader.pe.maxCoffSymbolNameLength")) + maxSymNameLen = settings->Get("loader.pe.maxCoffSymbolNameLength", this); + if (settings && settings->Contains("loader.pe.maxTotalCoffSymbolNameBytes")) + maxTotalSymNameBytes = settings->Get("loader.pe.maxTotalCoffSymbolNameBytes", this) + * 1024 * 1024; + // A name length limit of 0 means no limit; ReadCString takes an actual byte count, + // so map it to the largest representable value instead of reading zero bytes. + if (!maxSymNameLen) + maxSymNameLen = UINT64_MAX; + + // Preserve the original count for locating the string table, which sits immediately + // after all symbol table entries. Truncating coffSymbolCount for the loop must not + // affect the string table offset calculation. + // A limit of 0 disables the corresponding check. + uint32_t originalCoffSymbolCount = header.coffSymbolCount; + if (maxSymCount && header.coffSymbolCount > maxSymCount) + { + m_logger->LogWarn("COFF symbol count %u exceeds limit %" PRIu64 ", truncating.", + header.coffSymbolCount, maxSymCount); + header.coffSymbolCount = (uint32_t)maxSymCount; + } + BinaryReader stringReader(GetParentView(), LittleEndian); - uint64_t stringTableBase = header.coffSymbolTable + (header.coffSymbolCount * 18); + uint64_t stringTableBase = header.coffSymbolTable + ((uint64_t)originalCoffSymbolCount * 18); stringReader.Seek(stringTableBase); - if ((stringTableBase + stringReader.Read32()) > GetParentView()->GetEnd()) + uint32_t stringTableLen; + if (!stringReader.TryRead32(stringTableLen) || (stringTableBase + stringTableLen) > GetParentView()->GetEnd()) { throw PEFormatException("invalid COFF string table size"); } + // Symbol names are looked up by string table offset before being read, so entries + // that share an offset only pay for one read and count once toward the name budget. + std::unordered_map symbolNameCache; + uint64_t totalSymNameBytesRead = 0; for (size_t i = 0; i < header.coffSymbolCount; i++) { reader.Seek(header.coffSymbolTable + (i * 18)); @@ -1394,10 +1459,29 @@ bool PEView::Init() stringReader.Seek(header.coffSymbolTable + (i * 18)); symbolName = stringReader.ReadCString(8); } - else + else if (e_offset < stringTableLen) { - stringReader.Seek(stringTableBase + e_offset); - symbolName = stringReader.ReadCString(); + auto cached = symbolNameCache.find(e_offset); + if (cached != symbolNameCache.end()) + { + symbolName = cached->second; + } + else if (!maxTotalSymNameBytes || totalSymNameBytesRead < maxTotalSymNameBytes) + { + stringReader.Seek(stringTableBase + e_offset); + symbolName = stringReader.ReadCString(maxSymNameLen); + // Each name ends up retained in more than one copy once a symbol is + // created for it (raw, short, and full demangled forms), so weight + // the budget accordingly rather than counting only the bytes read here. + totalSymNameBytesRead += (uint64_t)symbolName.size() * 4; + symbolNameCache.emplace(e_offset, symbolName); + } + else + { + m_logger->LogWarn("Total COFF symbol name bytes exceeded limit %" PRIu64 + ", stopping symbol processing at index %zu.", maxTotalSymNameBytes, i); + break; + } } } @@ -3836,6 +3920,36 @@ Ref PEViewType::GetLoadSettingsForData(BinaryView* data) "description" : "Maximum number of resource directory tables to parse. This limit prevents infinite loops when processing malformed or malicious PE files with circular resource directory references." })"); + settings->RegisterSetting("loader.pe.maxCoffSymbolCount", + R"({ + "title" : "Maximum PE COFF Symbol Count", + "type" : "number", + "default" : 1000000, + "minValue" : 0, + "maxValue" : 100000000, + "description" : "Maximum number of COFF symbol table entries to process. Symbol counts above this are truncated. Set to 0 to disable this limit." + })"); + + settings->RegisterSetting("loader.pe.maxCoffSymbolNameLength", + R"({ + "title" : "Maximum PE COFF Symbol Name Length", + "type" : "number", + "default" : 32768, + "minValue" : 0, + "maxValue" : 1000000, + "description" : "Maximum number of bytes read for a single COFF symbol name from the string table. 32768 comfortably covers the longest real-world Rust mangled names. Set to 0 to disable this limit." + })"); + + settings->RegisterSetting("loader.pe.maxTotalCoffSymbolNameBytes", + R"json({ + "title" : "Maximum PE COFF Total Symbol Name Budget (MB)", + "type" : "number", + "default" : 1024, + "minValue" : 0, + "maxValue" : 10240, + "description" : "Maximum total memory (in MB) budgeted for all COFF symbol names combined. Set to 0 to disable this limit." + })json"); + return settings; } diff --git a/view/pe/peview.h b/view/pe/peview.h index cb933ef09..6ffe2b6e0 100644 --- a/view/pe/peview.h +++ b/view/pe/peview.h @@ -12,6 +12,15 @@ #define PE_ATTR_UNINIT_DATA 0x80 #define PE_ATTR_EXEC 0x20000000 +// The Windows loader always aligns PointerToRawData down to this boundary for PE32/PE32+. +#define PE_SECTION_RAW_DATA_ALIGNMENT 0x200u + +// Default values for the COFF symbol table loader settings, shared by peview.cpp and +// coffview.cpp. Keep these in sync with the "default" values in each RegisterSetting call. +#define PE_DEFAULT_MAX_COFF_SYMBOL_COUNT 1000000ULL +#define PE_DEFAULT_MAX_COFF_SYMBOL_NAME_LENGTH 32768ULL +#define PE_DEFAULT_MAX_TOTAL_COFF_SYMBOL_NAME_MB 1024ULL + // The dalay load table uses RVA, rather than VA #define PE_DLATTR_RVA 0x1 From 96f0a759c86e624bde3ee928278e0033c634eb12 Mon Sep 17 00:00:00 2001 From: weitao sun Date: Wed, 19 Aug 2026 12:33:15 -0400 Subject: [PATCH 2/4] peview: keep section virtualSize at least as large as sizeOfRawData Segment/section creation and RVA characteristics/symbol placement bound a section's length using two different fields: file-backed reads use sizeOfRawData, while everything else uses virtualSize. When a section's raw data extends past its declared virtual size, the extra bytes end up readable via RVA-to-file-offset translation but outside the mapped segment, section, and symbol range. Grow virtualSize to cover sizeOfRawData so all of these stay consistent. This applies regardless of PE32/PE32+ magic, since it isn't describing loader-specific alignment behavior, just keeping the view's own internal bookkeeping self-consistent. --- view/pe/peview.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/view/pe/peview.cpp b/view/pe/peview.cpp index dc9eda25d..4b2d8d0be 100644 --- a/view/pe/peview.cpp +++ b/view/pe/peview.cpp @@ -882,6 +882,14 @@ bool PEView::Init() { section.virtualSize = section.sizeOfRawData; } + // Segments, sections, RVA characteristics, and symbol placement are all bounded by + // virtualSize elsewhere in this file, while file-backed reads are bounded by + // sizeOfRawData. Keep virtualSize at least as large as sizeOfRawData so a section + // whose raw data extends past its declared virtual size is still fully mapped. + if (section.sizeOfRawData > section.virtualSize) + { + section.virtualSize = section.sizeOfRawData; + } m_sections.push_back(section); uint32_t flags = 0; From 4d7ec5a5f0cba743d6f3284dfb5acaefc95dcce2 Mon Sep 17 00:00:00 2001 From: weitao sun Date: Wed, 9 Sep 2026 17:09:19 -0400 Subject: [PATCH 3/4] peview: skip sector rounding for low-alignment PE images Section raw-data rounding to the 0x200 sector boundary only applies to normally page-aligned images. Per the PE spec, when SectionAlignment is below the architecture's page size, FileAlignment must equal SectionAlignment and raw offsets map directly to RVAs without the usual sector padding. Detect that case and skip both the PointerToRawData and SizeOfRawData rounding for it, so low-alignment images keep their declared file layout. Co-Authored-By: Claude Sonnet 5 --- view/pe/peview.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/view/pe/peview.cpp b/view/pe/peview.cpp index 4b2d8d0be..f139b83ad 100644 --- a/view/pe/peview.cpp +++ b/view/pe/peview.cpp @@ -697,6 +697,13 @@ bool PEView::Init() uint32_t resolvedFileAlignment = fileAlignmentValid ? opt.fileAlign : 0x200; if (!fileAlignmentValid) m_logger->LogWarn("PE has invalid FileAlignment with value: 0x%x", opt.fileAlign); + // Per the PE spec, when SectionAlignment is less than the architecture's page size, + // FileAlignment must equal SectionAlignment (both can legitimately be below the usual + // 0x200 sector size), and section raw data is mapped as declared rather than padded to + // sector boundaries. Detect that case so the section-level rounding below, which only + // applies to normally-aligned images, doesn't corrupt these low-alignment layouts. + uint32_t pageSize = (header.machine == IMAGE_FILE_MACHINE_IA64) ? 0x2000 : 0x1000; + bool lowAlignmentImage = opt.sectionAlign && (opt.sectionAlign < pageSize) && (opt.sectionAlign == opt.fileAlign); m_sizeOfHeaders = opt.sizeOfHeaders; if (opt.sizeOfHeaders % resolvedFileAlignment) m_sizeOfHeaders = (opt.sizeOfHeaders + resolvedFileAlignment) & ~(resolvedFileAlignment - 1); @@ -839,8 +846,10 @@ bool PEView::Init() section.pointerToRawData = reader.Read32(); // Windows always rounds PointerToRawData down to a 0x200 boundary for PE32/PE32+, // regardless of the FileAlignment field value. Apply the same behavior here so that - // our view matches what the Windows loader actually maps into memory. - if ((opt.magic == 0x10b || opt.magic == 0x20b) && (section.pointerToRawData & (PE_SECTION_RAW_DATA_ALIGNMENT - 1))) + // our view matches what the Windows loader actually maps into memory. Low-alignment + // images are the documented exception: skip the rounding so file offsets keep + // matching RVAs as declared. + if (!lowAlignmentImage && (opt.magic == 0x10b || opt.magic == 0x20b) && (section.pointerToRawData & (PE_SECTION_RAW_DATA_ALIGNMENT - 1))) { m_logger->LogWarn("PE section[%u]: pointerToRawData 0x%x is not 0x200-aligned, " "rounding down to 0x%x per Windows loader behavior.", @@ -849,9 +858,11 @@ bool PEView::Init() } // Windows rounds SizeOfRawData up to the nearest FileAlignment multiple for PE32/PE32+. // Without this, bytes between the raw value and the rounded value are invisible to - // analysis even though the Windows loader maps them. + // analysis even though the Windows loader maps them. Skip this for low-alignment + // images for the same reason as the PointerToRawData rounding above. // Cap at the remaining file bytes to avoid mapping data past the end of the file. - if ((opt.magic == 0x10b || opt.magic == 0x20b) + if (!lowAlignmentImage + && (opt.magic == 0x10b || opt.magic == 0x20b) && section.sizeOfRawData && (section.sizeOfRawData % resolvedFileAlignment)) { From bc9cad87700d7349002036bc9b58a11bb3f57e3b Mon Sep 17 00:00:00 2001 From: weitao sun Date: Thu, 10 Sep 2026 18:02:06 -0400 Subject: [PATCH 4/4] peview/coffview: Fix COFF symbol budget, relocation, and string table bounds - Charge every retained COFF symbol name against the total-name-bytes budget, including cache hits. - Resolve external symbol names on demand in the relocation loop for symbol table entries outside the per-file annotation limit, sharing the same budget and cache as the initial pass. - Make RVAToFileOffset and GetRVACharacteristics prefer the most recently added section when sections overlap, matching the rest of the loader. - Require the string table length to be at least 4 bytes, validate name offsets against the table bounds, and cap name reads to the bytes remaining in the table, across all three name lookups in these files (COFF symbol names, PE symbol names, COFF section names). Co-Authored-By: Claude Sonnet 5 --- view/pe/coffview.cpp | 187 ++++++++++++++++++++++++++++++++----------- view/pe/peview.cpp | 61 ++++++++++---- 2 files changed, 184 insertions(+), 64 deletions(-) diff --git a/view/pe/coffview.cpp b/view/pe/coffview.cpp index 6f88ff481..17e1038df 100644 --- a/view/pe/coffview.cpp +++ b/view/pe/coffview.cpp @@ -264,16 +264,22 @@ bool COFFView::Init() uint64_t stringTableBase = header.coffSymbolTable + ((uint64_t)header.coffSymbolCount * sizeofCOFFSymbol); stringReader.Seek(stringTableBase); uint32_t stringTableLen = stringReader.Read32(); - if ((stringTableBase + stringTableLen) > GetParentView()->GetEnd()) + // The first 4 bytes of the string table are the length field itself, so a table + // shorter than that can't hold even its own header; the rest must fit in the file. + if (stringTableLen < 4 || (stringTableBase + stringTableLen) > GetParentView()->GetEnd()) { m_logger->LogError("Cannot resolve section name \"%s\": String table is invalid length", name); } - else if (offset < stringTableLen) + // Payload offsets start after the 4-byte length field; offsets inside it don't + // name a string. + else if (offset >= 4 && offset < stringTableLen) { sectionNameReader.Seek(stringTableBase + offset); - // Section names longer than 1024 bytes are not meaningful; cap the read - // to bound the allocation. - resolvedName = sectionNameReader.ReadCString(1024); + // Section names longer than 1024 bytes are not meaningful; cap the read to + // bound the allocation, and to what's left in the table so a name lacking a + // null terminator can't run past the table's declared end. + uint64_t remaining = stringTableLen - offset; + resolvedName = sectionNameReader.ReadCString(std::min(1024, remaining)); } else { @@ -695,6 +701,77 @@ bool COFFView::Init() // The offset of the symbol table after adjusting for the alignment of the sections that precede it uint64_t symbolTableAdjustedOffset = 0; + // Symbol-name resolution and its retained-bytes budget are shared between the initial + // pass over the symbol table below and the relocation pass further down, which needs to + // resolve a name on demand for an entry whose annotation was skipped the first time + // through -- both must respect the same budget so the lazy path can't bypass it. + BinaryReader stringReader(GetParentView(), LittleEndian); + std::unordered_map symbolNameCache; + uint64_t totalSymNameBytesRead = 0; + bool nameBudgetExceeded = false; + uint64_t maxSymNameLen = PE_DEFAULT_MAX_COFF_SYMBOL_NAME_LENGTH; + uint64_t maxTotalSymNameBytes = PE_DEFAULT_MAX_TOTAL_COFF_SYMBOL_NAME_MB * 1024 * 1024; + uint64_t stringTableBaseRaw = 0; + uint32_t stringTableSize = 0; + // Tracks undefined external symbols resolved lazily by the relocation pass (see + // below), keyed by symbol-table index, so relocations sharing an index only pay for + // the resolution and symbol creation once. + std::unordered_map lazyExternalSymbolNames; + + // Resolves a symbol's name from its short (embedded) or long (string-table) form. + // Shared by the initial pass over the symbol table below and by the relocation pass + // further down, which needs to resolve a name on demand for an entry whose annotation + // was skipped the first time through. Every returned name counts against the budget, + // including cache hits, since each caller retains its own copy of it. + auto resolveSymbolName = [&](size_t idx, uint32_t zeroes, uint32_t offset) -> string + { + if (zeroes) + { + stringReader.Seek(header.coffSymbolTable + (idx * sizeofCOFFSymbol)); + string name = stringReader.ReadCString(8); + return name.substr(0, strlen(name.c_str())); + } + // Payload offsets start after the 4-byte length field; offsets inside it don't + // name a string. + if (nameBudgetExceeded || offset < 4 || offset >= stringTableSize) + return string(); + auto cached = symbolNameCache.find(offset); + if (cached != symbolNameCache.end()) + { + uint64_t projected = totalSymNameBytesRead + (uint64_t)cached->second.size() * 4; + if (maxTotalSymNameBytes && projected > maxTotalSymNameBytes) + { + m_logger->LogWarn("Total COFF symbol name bytes exceeded limit %" PRIu64 + ", limiting further symbol name resolution.", maxTotalSymNameBytes); + nameBudgetExceeded = true; + return string(); + } + totalSymNameBytesRead = projected; + return cached->second; + } + // Cap the read to what's left in the table so a name lacking a null terminator + // can't run past the table's declared end. + uint64_t remaining = stringTableSize - offset; + uint64_t cap = std::min(maxSymNameLen, remaining); + stringReader.Seek(stringTableBaseRaw + offset); + string name = stringReader.ReadCString(cap); + // Each name ends up retained in more than one copy once a symbol is created for + // it, so weight the budget accordingly. Every symbol that retains a reference + // counts toward it, including ones that hit the cache above, since each still + // gets its own retained copies downstream — only the read itself is deduplicated. + uint64_t projected = totalSymNameBytesRead + (uint64_t)name.size() * 4; + if (maxTotalSymNameBytes && projected > maxTotalSymNameBytes) + { + m_logger->LogWarn("Total COFF symbol name bytes exceeded limit %" PRIu64 + ", limiting further symbol name resolution.", maxTotalSymNameBytes); + nameBudgetExceeded = true; + return string(); + } + totalSymNameBytesRead = projected; + symbolNameCache.emplace(offset, name); + return name; + }; + try { // Process COFF symbol table @@ -873,8 +950,6 @@ bool COFFView::Init() // A limit of 0 disables the corresponding check. uint64_t maxSymCount = PE_DEFAULT_MAX_COFF_SYMBOL_COUNT; - uint64_t maxSymNameLen = PE_DEFAULT_MAX_COFF_SYMBOL_NAME_LENGTH; - uint64_t maxTotalSymNameBytes = PE_DEFAULT_MAX_TOTAL_COFF_SYMBOL_NAME_MB * 1024 * 1024; if (settings && settings->Contains("loader.coff.maxCoffSymbolCount")) maxSymCount = settings->Get("loader.coff.maxCoffSymbolCount", this); if (settings && settings->Contains("loader.coff.maxCoffSymbolNameLength")) @@ -914,12 +989,13 @@ bool COFFView::Init() DefineDataVariable(coffSymbolTableBase, Type::ArrayType(Type::NamedType(this, coffSymbolName), header.coffSymbolCount)); DefineAutoSymbol(new Symbol(DataSymbol, "__symtab", coffSymbolTableBase, NoBinding)); - BinaryReader stringReader(GetParentView(), LittleEndian); - uint64_t stringTableBaseRaw = header.coffSymbolTable + ((uint64_t)header.coffSymbolCount * sizeofCOFFSymbol); + stringTableBaseRaw = header.coffSymbolTable + ((uint64_t)header.coffSymbolCount * sizeofCOFFSymbol); stringReader.Seek(stringTableBaseRaw); - uint32_t stringTableSize; - if (!stringReader.TryRead32(stringTableSize) || (stringTableBaseRaw + stringTableSize) > GetParentView()->GetEnd()) + // The first 4 bytes of the string table are the length field itself, so a table + // shorter than that can't hold even its own header; the rest must fit in the file. + if (!stringReader.TryRead32(stringTableSize) || stringTableSize < 4 + || (stringTableBaseRaw + stringTableSize) > GetParentView()->GetEnd()) { throw COFFFormatException("invalid COFF string table size"); } @@ -942,11 +1018,6 @@ bool COFFView::Init() DefineAutoSymbol(new Symbol(DataSymbol, "__strtab", m_imageBase + stringTableBase + 4, NoBinding)); - // Symbol names are looked up by string table offset before being read, so entries - // that share an offset only pay for one read and count once toward the name budget. - std::unordered_map symbolNameCache; - uint64_t totalSymNameBytesRead = 0; - bool nameBudgetExceeded = false; for (size_t i = 0; i < header.coffSymbolCount; i++) { // Every slot still gets the marker symbol below regardless of this limit, so @@ -976,38 +1047,12 @@ bool COFFView::Init() break; } - // read symbol name + // read symbol name. The short (embedded) form is always resolved — it's a + // fixed-size read straight out of the symbol record, not string-table I/O — + // while the long form is bounded by the annotation limit. string symbolName; - if (e_zeroes) - { - stringReader.Seek(header.coffSymbolTable + (i * sizeofCOFFSymbol)); - symbolName = stringReader.ReadCString(8); - symbolName = symbolName.substr(0, strlen(symbolName.c_str())); - } - else if (annotate && e_offset < stringTableSize) - { - auto cached = symbolNameCache.find(e_offset); - if (cached != symbolNameCache.end()) - { - symbolName = cached->second; - } - else if (!maxTotalSymNameBytes || totalSymNameBytesRead < maxTotalSymNameBytes) - { - stringReader.Seek(stringTableBaseRaw + e_offset); - symbolName = stringReader.ReadCString(maxSymNameLen); - // Each name ends up retained in more than one copy once a symbol is - // created for it, so weight the budget accordingly rather than counting - // only the bytes read here. - totalSymNameBytesRead += (uint64_t)symbolName.size() * 4; - symbolNameCache.emplace(e_offset, symbolName); - } - else - { - m_logger->LogWarn("Total COFF symbol name bytes exceeded limit %" PRIu64 - ", limiting further symbol name resolution.", maxTotalSymNameBytes); - nameBudgetExceeded = true; - } - } + if (e_zeroes || annotate) + symbolName = resolveSymbolName(i, e_zeroes, e_offset); BNSymbolBinding binding; bool clrFunction = false; @@ -1082,7 +1127,10 @@ bool COFFView::Init() string symbolStructName = "__symbol(" + symbolName + ")"; DefineAutoSymbol(new Symbol(DataSymbol, symbolStructName, m_imageBase + symbolVirtualAddress, NoBinding)); - if (annotate && e_zeroes == 0 && e_offset < stringTableSize) + // Tie this to whether a name was actually resolved (empty when the offset + // was invalid or the budget was already exceeded) rather than re-deriving + // the same bounds check independently. + if (annotate && e_zeroes == 0 && !symbolName.empty()) { DefineDataVariable(m_imageBase + stringTableBase + e_offset, Type::ArrayType(Type::IntegerType(1, true, "char"), symbolName.length() + 1)); string symbolStringName = "__symbol_name(" + symbolName + ")"; @@ -1412,6 +1460,49 @@ bool COFFView::Init() if (targetSymbol) break; } + // The marker's embedded name is only populated when this slot was + // within the annotation limit during the initial pass; entries + // beyond it never got an ExternalSymbol, so the lookup above finds + // nothing even though the underlying symbol is real. A relocation + // actually needing this symbol is reason enough to resolve it now + // and create it on demand. Resolved once per symbol-table index and + // cached, so relocations sharing an index don't repeat the creation + // work — the added cost is bounded by how many *distinct* undefined + // external symbols relocations reference, not by relocation count or + // the file's declared symbol count. + if (!targetSymbol && coffSymbol.value == 0 + && (!isBigCOFF ? coffSymbol.sectionNumber.i16 : coffSymbol.sectionNumber.i32) == IMAGE_SYM_UNDEFINED) + { + string lazyName; + auto lazyCached = lazyExternalSymbolNames.find(symbolTableIndex); + if (lazyCached != lazyExternalSymbolNames.end()) + { + lazyName = lazyCached->second; + } + else + { + reader.Seek(header.coffSymbolTable + (symbolTableIndex * sizeofCOFFSymbol)); + uint32_t lazyZeroes = reader.Read32(); + uint32_t lazyOffset = reader.Read32(); + lazyName = resolveSymbolName(symbolTableIndex, lazyZeroes, lazyOffset); + if (!lazyName.empty()) + AddCOFFSymbol(ExternalSymbol, "", lazyName, symbolOffset); + lazyExternalSymbolNames.emplace(symbolTableIndex, lazyName); + } + if (!lazyName.empty()) + { + for (const auto& externSymbol : GetSymbolsByName(lazyName)) + { + auto type = externSymbol->GetType(); + if (type == ExternalSymbol || type == ImportedFunctionSymbol || type == ImportedDataSymbol || type == ImportAddressSymbol) + { + targetSymbol = externSymbol; + DefineRelocation(m_arch, reloc, targetSymbol, m_imageBase + reloc.address); + break; + } + } + } + } if (! targetSymbol) { // TODO: determine whether this is actually worth logging -- may only be happening for NB (non-based) relocations? diff --git a/view/pe/peview.cpp b/view/pe/peview.cpp index f139b83ad..eceb5dd08 100644 --- a/view/pe/peview.cpp +++ b/view/pe/peview.cpp @@ -1436,13 +1436,17 @@ bool PEView::Init() uint64_t stringTableBase = header.coffSymbolTable + ((uint64_t)originalCoffSymbolCount * 18); stringReader.Seek(stringTableBase); uint32_t stringTableLen; - if (!stringReader.TryRead32(stringTableLen) || (stringTableBase + stringTableLen) > GetParentView()->GetEnd()) + // The first 4 bytes of the string table are the length field itself, so a table + // shorter than that can't hold even its own header; the rest must fit in the file. + if (!stringReader.TryRead32(stringTableLen) || stringTableLen < 4 + || (stringTableBase + stringTableLen) > GetParentView()->GetEnd()) { throw PEFormatException("invalid COFF string table size"); } // Symbol names are looked up by string table offset before being read, so entries - // that share an offset only pay for one read and count once toward the name budget. + // that share an offset only pay for one read; every symbol that retains a + // reference to a name still counts toward the budget, cached or not. std::unordered_map symbolNameCache; uint64_t totalSymNameBytesRead = 0; for (size_t i = 0; i < header.coffSymbolCount; i++) @@ -1478,29 +1482,42 @@ bool PEView::Init() stringReader.Seek(header.coffSymbolTable + (i * 18)); symbolName = stringReader.ReadCString(8); } - else if (e_offset < stringTableLen) + // Payload offsets start after the 4-byte length field; offsets inside it don't + // name a string. + else if (e_offset >= 4 && e_offset < stringTableLen) { auto cached = symbolNameCache.find(e_offset); + string candidate; if (cached != symbolNameCache.end()) { - symbolName = cached->second; + candidate = cached->second; } - else if (!maxTotalSymNameBytes || totalSymNameBytesRead < maxTotalSymNameBytes) + else { + // Cap the read to what's left in the table so a name lacking a null + // terminator can't run past the table's declared end. + uint64_t remaining = stringTableLen - e_offset; + uint64_t cap = std::min(maxSymNameLen, remaining); stringReader.Seek(stringTableBase + e_offset); - symbolName = stringReader.ReadCString(maxSymNameLen); - // Each name ends up retained in more than one copy once a symbol is - // created for it (raw, short, and full demangled forms), so weight - // the budget accordingly rather than counting only the bytes read here. - totalSymNameBytesRead += (uint64_t)symbolName.size() * 4; - symbolNameCache.emplace(e_offset, symbolName); + candidate = stringReader.ReadCString(cap); } - else + + // Each name ends up retained in more than one copy once a symbol is + // created for it (raw, short, and full demangled forms), so weight the + // budget accordingly. Every symbol that retains a reference counts toward + // it, including ones that hit the cache above, since each still gets its + // own retained copies downstream — only the read itself is deduplicated. + uint64_t projected = totalSymNameBytesRead + (uint64_t)candidate.size() * 4; + if (maxTotalSymNameBytes && projected > maxTotalSymNameBytes) { m_logger->LogWarn("Total COFF symbol name bytes exceeded limit %" PRIu64 ", stopping symbol processing at index %zu.", maxTotalSymNameBytes, i); break; } + totalSymNameBytesRead = projected; + symbolName = candidate; + if (cached == symbolNameCache.end()) + symbolNameCache.emplace(e_offset, candidate); } } @@ -3627,16 +3644,25 @@ bool PEView::Init() uint64_t PEView::RVAToFileOffset(uint64_t offset, bool except) { + // Sections can overlap (declared that way in the file, or made to by sector rounding + // above), in which case the most recently added one wins for the bytes the BinaryView + // actually maps. Scan the whole list rather than stopping at the first match so this + // picks the same section core does, instead of always favoring the earliest one. + bool found = false; + uint64_t result = 0; for (auto& i : m_sections) { if ((offset >= i.virtualAddress) && (offset < (i.virtualAddress + i.sizeOfRawData)) && (i.virtualSize != 0)) { - uint64_t progOfs = offset - i.virtualAddress; - return i.pointerToRawData + progOfs; + result = i.pointerToRawData + (offset - i.virtualAddress); + found = true; } } + if (found) + return result; + if (!except) return offset; @@ -3646,12 +3672,15 @@ uint64_t PEView::RVAToFileOffset(uint64_t offset, bool except) uint32_t PEView::GetRVACharacteristics(uint64_t offset) { + // See the matching comment in RVAToFileOffset: keep the last match, not the first, so + // this agrees with which section's bytes are actually mapped when sections overlap. + uint32_t result = 0; for (auto& i : m_sections) { if ((offset >= i.virtualAddress) && (offset < (i.virtualAddress + i.virtualSize)) && (i.virtualSize != 0)) - return i.characteristics; + result = i.characteristics; } - return 0; + return result; }