PE COFF Debug symbols are parsed incorrectly

[d0mnik]

Version and Platform (required):

• Binary Ninja Version: 3.5.4285-dev
• OS: MacOS
• OS Version: Ventura
• CPU Architecture: M1

Bug Description:
In IDA, function names are automatically being discovered (not FLIRT) that BN does not pick up.
BN picks up some of the function names, but misses out on a few that was picked out by IDA.

Steps To Reproduce:
Please provide all steps required to reproduce the behavior:
Load the following binary in both IDA & Binary Ninja.
binary.zip

Expected Behavior:
In this particular case, a function at address 0x57db80 should be named as _FXCLI_DebugDispatch.

Screenshots:

In IDA:

In Binary Ninja:

Notice how the function at 0x57db80is named as _FXCLI_DebugDispatch in IDA, but not in Binary Ninja

[negasora]

We're create _FXCLI_DebugDispatch at 0x57eb80 instead of 0x57db80

[d0mnik]

I see that in the symbol UI as well, however the function defined at 0x57eb80 is never called(?)
I also noticed that it does not look like a typical function as well, and its just laying in the middle of another function 0x57eb01 (_FXCLI_IF_NotifyOraBR)

[plafosse]

Seems like when adding support for this: #1956 we might have implemented support incorrectly/incompletely
Potentially more correct implementation: https://github.com/trailofbits/pe-parse/blob/ac6a07bdbf4034cd3977914fc6a1964ae65eff01/pe-parser-library/src/parse.cpp#L2289

[negasora]

The symbol table entry for _FXCLI_DebugDispatch has storage class IMAGE_SYM_CLASS_EXTERNAL, and it's stated here:

"If the section number is not zero, then the Value field specifies the offset within the section."

The section number of the entry is 1, pointing to .text, which starts at 0x1000
The value field is 0x57db80

So it makes sense that the symbol is at 0x57eb80
(Also https://crates.io/crates/object and ghidra both agree with us that _FXCLI_DebugDispatch is at 0x57eb80)

The only thing I can think of that could explain this is if "section number is not zero" actually means "If the section number is not one", since the section number in the entry is one indexed.

[plafosse]

I've tracked down the issue. This is an old VC++ 6.0 Binary with /DEBUGTYPE:COFF and so the values are already RVAs. We need to detect this condition and not add the section offset. I've created a PR to address this.