Fix legacy PE COFF symbol addresses

view/pe/peview.cpp

@@ -1358,6 +1358,7 @@ bool PEView::Init()
 		// Process COFF symbol table
 		if (header.coffSymbolCount)
 		{
+			const bool coffSymbolValuesAreRvas = CoffSymbolValuesAreRvas(header);
 			BinaryReader stringReader(GetParentView(), LittleEndian);
 			uint64_t stringTableBase = header.coffSymbolTable + (header.coffSymbolCount * 18);
 			stringReader.Seek(stringTableBase);
@@ -1386,7 +1387,8 @@ bool PEView::Init()
 						break;
 					default:
 						if (size_t(e_scnum - 1) < m_sections.size())
-							virtualAddress = m_sections[size_t(e_scnum - 1)].virtualAddress + e_value;
+							virtualAddress = coffSymbolValuesAreRvas ? e_value :
+								m_sections[size_t(e_scnum - 1)].virtualAddress + e_value;
 						break;
 				}
 
@@ -3440,6 +3442,91 @@ uint64_t PEView::RVAToFileOffset(uint64_t offset, bool except)
 }
 
 
+bool PEView::CoffSymbolValuesAreRvas(const PEHeader& header)
+{
+	/*
+	 * A normal PE COFF symbol table stores section-relative symbol values. Legacy
+	 * link.exe /DEBUGTYPE:COFF output instead wraps the table in an
+	 * IMAGE_COFF_SYMBOLS_HEADER and stores image-relative values. Keep the normal
+	 * interpretation used for https://github.com/Vector35/binaryninja-api/issues/1956,
+	 * and select the legacy interpretation only when the wrapper exactly identifies
+	 * the PE header's table. See:
+	 *
+	 * https://github.com/Vector35/binaryninja-api/issues/4308
+	 * https://learn.microsoft.com/en-us/archive/msdn-magazine/2002/march/inside-windows-an-in-depth-look-into-the-win32-portable-executable-file-format-part-2
+	 * https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-image_coff_symbols_header
+	 * https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#debug-directory-image-only
+	 *
+	 * AddressOfRawData is allowed to be zero for debug data outside an image
+	 * section, so this check deliberately uses PointerToRawData.
+	 */
+	if (!header.coffSymbolTable || !header.coffSymbolCount ||
+		(m_dataDirs.size() <= IMAGE_DIRECTORY_ENTRY_DEBUG))
+		return false;
+
+	const PEDataDirectory& dir = m_dataDirs[IMAGE_DIRECTORY_ENTRY_DEBUG];
+	if (!dir.virtualAddress || (dir.size < sizeof(DebugDirectory)))
+		return false;
+
+	const uint64_t fileEnd = GetParentView()->GetEnd();
+	uint64_t debugDirectoryOffset;
+	try
+	{
+		debugDirectoryOffset = RVAToFileOffset(dir.virtualAddress);
+	}
+	catch (const std::exception&)
+	{
+		return false;
+	}
+
+	const uint64_t debugDirectorySize = dir.size - (dir.size % sizeof(DebugDirectory));
+	if ((debugDirectoryOffset > fileEnd) || (debugDirectorySize > fileEnd - debugDirectoryOffset))
+		return false;
+
+	BinaryReader reader(GetParentView(), LittleEndian);
+	for (uint64_t offset = 0; offset < debugDirectorySize; offset += sizeof(DebugDirectory))
+	{
+		reader.Seek(debugDirectoryOffset + offset);
+		reader.Read32();  // Characteristics
+		reader.Read32();  // TimeDateStamp
+		reader.Read16();  // MajorVersion
+		reader.Read16();  // MinorVersion
+		const uint32_t type = reader.Read32();
+		const uint32_t sizeOfData = reader.Read32();
+		reader.Read32();  // AddressOfRawData
+		const uint32_t pointerToRawData = reader.Read32();
+
+		constexpr uint64_t coffSymbolsHeaderSize = 8 * sizeof(uint32_t);
+		if ((type != IMAGE_DEBUG_TYPE_COFF) || !pointerToRawData ||
+			(sizeOfData < coffSymbolsHeaderSize) || (pointerToRawData > fileEnd) ||
+			(sizeOfData > fileEnd - pointerToRawData))
+			continue;
+
+		reader.Seek(pointerToRawData);
+		const uint32_t numberOfSymbols = reader.Read32();
+		const uint32_t lvaToFirstSymbol = reader.Read32();
+		reader.Read32();  // NumberOfLinenumbers
+		reader.Read32();  // LvaToFirstLinenumber
+		reader.Read32();  // RvaToFirstByteOfCode
+		reader.Read32();  // RvaToLastByteOfCode
+		reader.Read32();  // RvaToFirstByteOfData
+		reader.Read32();  // RvaToLastByteOfData
+
+		const uint64_t symbolBytes = uint64_t(numberOfSymbols) * 18;
+		if ((numberOfSymbols != header.coffSymbolCount) ||
+			(uint64_t(pointerToRawData) + lvaToFirstSymbol != header.coffSymbolTable) ||
+			(lvaToFirstSymbol < coffSymbolsHeaderSize) || (lvaToFirstSymbol > sizeOfData) ||
+			(symbolBytes > sizeOfData - lvaToFirstSymbol))
+			continue;
+
+		m_logger->LogDebug("Using image-relative values from legacy COFF debug symbol table");
+		return true;
+	}
+
+	return false;
+}
+
+
 uint32_t PEView::GetRVACharacteristics(uint64_t offset)
 {
 	for (auto& i : m_sections)

view/pe/peview.h

@@ -466,6 +466,7 @@ namespace BinaryNinja
 		Ref<Metadata> m_symExternMappingMetadata;
 
 		uint64_t RVAToFileOffset(uint64_t rva, bool except = true);
+		bool CoffSymbolValuesAreRvas(const PEHeader& header);
 		uint32_t GetRVACharacteristics(uint64_t rva);
 		std::string ReadString(uint64_t rva);
 		uint16_t Read16(uint64_t rva);

view/pe/tests/fixtures/README.md

@@ -0,0 +1,21 @@
+# Legacy COFF debug fixture
+
+`legacy_coff_debug.exe` is a minimal synthetic PE32 image for
+[binaryninja-api#4308](https://github.com/Vector35/binaryninja-api/issues/4308).
+It contains a raw-only `IMAGE_DEBUG_TYPE_COFF` entry and an
+`IMAGE_COFF_SYMBOLS_HEADER` whose symbol table matches the PE file header.
+
+The `legacy` function is physically located at RVA `0x1010`, and its COFF
+symbol value is also `0x1010`. The separate PE entry point is at RVA `0x1020`,
+so Binary Ninja's automatic `_start` symbol does not obscure `legacy` in the
+UI. Treating the record as an ordinary section-relative symbol incorrectly
+adds the `.text` RVA of `0x1000` and places the symbol at RVA `0x2010`.
+
+Regenerate the fixture with:
+
+```sh
+python3 generate_legacy_coff_debug.py legacy_coff_debug.exe
+```
+
+Expected SHA-256:
+`7d3d2b3a45405e542d4c5644712cd11b27d5c08b968f7020e2960ecffb8dcba7`.

view/pe/tests/fixtures/generate_legacy_coff_debug.py

@@ -0,0 +1,223 @@
+#!/usr/bin/env python3
+"""Generate a minimal PE32 image with a legacy /DEBUGTYPE:COFF table."""
+
+import argparse
+import struct
+from pathlib import Path
+
+
+FILE_ALIGNMENT = 0x200
+SECTION_ALIGNMENT = 0x1000
+IMAGE_BASE = 0x400000
+
+PE_OFFSET = 0x80
+TEXT_RVA = 0x1000
+TEXT_RAW_OFFSET = 0x200
+TEXT_RAW_SIZE = 0x1200
+TARGET_RVA = 0x1010
+ENTRY_RVA = 0x1020
+
+RDATA_RVA = 0x3000
+RDATA_RAW_OFFSET = 0x1400
+RDATA_RAW_SIZE = 0x200
+
+DEBUG_DIRECTORY_RVA = RDATA_RVA
+DEBUG_DIRECTORY_SIZE = 28
+COFF_DEBUG_OFFSET = 0x1600
+COFF_HEADER_SIZE = 32
+COFF_SYMBOL_OFFSET = COFF_DEBUG_OFFSET + COFF_HEADER_SIZE
+COFF_SYMBOL_COUNT = 1
+COFF_SYMBOL_SIZE = 18
+COFF_STRING_TABLE_SIZE = 4
+COFF_DEBUG_SIZE = COFF_HEADER_SIZE + COFF_SYMBOL_SIZE + COFF_STRING_TABLE_SIZE
+FILE_SIZE = COFF_DEBUG_OFFSET + COFF_DEBUG_SIZE
+
+
+def write_section_header(
+    image,
+    offset,
+    name,
+    virtual_size,
+    virtual_address,
+    raw_size,
+    raw_offset,
+    characteristics,
+):
+    struct.pack_into(
+        "<8sIIIIIIHHI",
+        image,
+        offset,
+        name.encode("ascii").ljust(8, b"\0"),
+        virtual_size,
+        virtual_address,
+        raw_size,
+        raw_offset,
+        0,
+        0,
+        0,
+        0,
+        characteristics,
+    )
+
+
+def build_image():
+    image = bytearray(FILE_SIZE)
+
+    image[0:2] = b"MZ"
+    struct.pack_into("<I", image, 0x3C, PE_OFFSET)
+
+    # PE signature and IMAGE_FILE_HEADER.
+    image[PE_OFFSET : PE_OFFSET + 4] = b"PE\0\0"
+    coff_offset = PE_OFFSET + 4
+    struct.pack_into(
+        "<HHIIIHH",
+        image,
+        coff_offset,
+        0x14C,  # IMAGE_FILE_MACHINE_I386
+        2,
+        0,
+        COFF_SYMBOL_OFFSET,
+        COFF_SYMBOL_COUNT,
+        0xE0,
+        0x0103,  # executable, relocations stripped, 32-bit
+    )
+
+    # IMAGE_OPTIONAL_HEADER32 through NumberOfRvaAndSizes.
+    optional_offset = coff_offset + 20
+    struct.pack_into(
+        "<HBBIIIIIIIIIHHHHHHIIIIHHIIIIII",
+        image,
+        optional_offset,
+        0x10B,
+        6,
+        0,
+        TEXT_RAW_SIZE,
+        RDATA_RAW_SIZE,
+        0,
+        ENTRY_RVA,
+        TEXT_RVA,
+        RDATA_RVA,
+        IMAGE_BASE,
+        SECTION_ALIGNMENT,
+        FILE_ALIGNMENT,
+        4,
+        0,
+        0,
+        0,
+        4,
+        0,
+        0,
+        0x4000,
+        FILE_ALIGNMENT,
+        0,
+        3,  # IMAGE_SUBSYSTEM_WINDOWS_CUI
+        0,
+        0x100000,
+        0x1000,
+        0x100000,
+        0x1000,
+        0,
+        16,
+    )
+
+    directories_offset = optional_offset + 96
+    struct.pack_into(
+        "<II",
+        image,
+        directories_offset + 6 * 8,
+        DEBUG_DIRECTORY_RVA,
+        DEBUG_DIRECTORY_SIZE,
+    )
+
+    section_offset = optional_offset + 0xE0
+    write_section_header(
+        image,
+        section_offset,
+        ".text",
+        0x1100,
+        TEXT_RVA,
+        TEXT_RAW_SIZE,
+        TEXT_RAW_OFFSET,
+        0x60000020,  # code, execute, read
+    )
+    write_section_header(
+        image,
+        section_offset + 40,
+        ".rdata",
+        DEBUG_DIRECTORY_SIZE,
+        RDATA_RVA,
+        RDATA_RAW_SIZE,
+        RDATA_RAW_OFFSET,
+        0x40000040,  # initialized data, read
+    )
+
+    # mov eax, 42; ret
+    target_offset = TEXT_RAW_OFFSET + TARGET_RVA - TEXT_RVA
+    image[target_offset : target_offset + 6] = b"\xB8\x2A\x00\x00\x00\xC3"
+    # xor eax, eax; ret -- keep the PE entry point distinct from `legacy` so
+    # Binary Ninja's automatic `_start` symbol does not hide it in the UI.
+    entry_offset = TEXT_RAW_OFFSET + ENTRY_RVA - TEXT_RVA
+    image[entry_offset : entry_offset + 3] = b"\x31\xC0\xC3"
+
+    # IMAGE_DEBUG_DIRECTORY. The payload is deliberately raw-only.
+    struct.pack_into(
+        "<IIHHIIII",
+        image,
+        RDATA_RAW_OFFSET,
+        0,
+        0,
+        0,
+        0,
+        1,  # IMAGE_DEBUG_TYPE_COFF
+        COFF_DEBUG_SIZE,
+        0,  # AddressOfRawData
+        COFF_DEBUG_OFFSET,
+    )
+
+    # IMAGE_COFF_SYMBOLS_HEADER.
+    struct.pack_into(
+        "<IIIIIIII",
+        image,
+        COFF_DEBUG_OFFSET,
+        COFF_SYMBOL_COUNT,
+        COFF_HEADER_SIZE,
+        0,
+        0,
+        TEXT_RVA,
+        TEXT_RVA + 0x1100,
+        0,
+        0,
+    )
+
+    # IMAGE_SYMBOL.Value is already an RVA. Adding .text's RVA again produces
+    # the incorrect address 0x402010 instead of 0x401010.
+    struct.pack_into(
+        "<8sIhHBB",
+        image,
+        COFF_SYMBOL_OFFSET,
+        b"legacy\0\0",
+        TARGET_RVA,
+        1,
+        0x20,  # IMAGE_SYM_DTYPE_FUNCTION
+        2,  # IMAGE_SYM_CLASS_EXTERNAL
+        0,
+    )
+    struct.pack_into(
+        "<I",
+        image,
+        COFF_SYMBOL_OFFSET + COFF_SYMBOL_SIZE,
+        COFF_STRING_TABLE_SIZE,
+    )
+
+    return image
+
+
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument("output", type=Path)
+    args = parser.parse_args()
+    args.output.write_bytes(build_image())
+
+
+if __name__ == "__main__":
+    main()