for i in 0..symbol_count {

This is wrong because it will try to parse auxiliary symbols as regular symbols. Use symbols.iter() instead.

First of all, there should not be a difference when running debug and release

Agreed. name_offset should use checked_sub instead so that we gracefully handle malformed files, but once you fix the iteration it won't matter for the file you are testing on.

from goblin.

Hmmm, thank you for the detailed writeup! Can you paste the binary by any chance with your fibonnaci function? I'm a little surprised this wouldn't have come up earlier, but it is possibly a regression since I know a change went in recently with pe and string table

from goblin.

Yes of course!

Here is the source code if you want to compile it yourself (but the binary is also attached, see below):

fn main() {
    println!("Here are some fibonacci numbers");
    println!("Fib 50: {}", fibonacci(50));
}

#[no_mangle]
fn fibonacci(n: u64) -> u64 {
    println!("addr of fib: {:#?}", fibonacci as *const u8);
    let mut a = 0;
    let mut b = 1;

    if n == 0 {
        return 0;
    }

    for _ in 1..n {
        let tmp = a + b;
        a = b;
        b = tmp;
    }

    b
}

(The program was called hash-test, but I stripped it of any meaningful computation. All that is left is a main function that calls a fibonacci calculation)
hash-test.zip

from goblin.

if you git revert eaba4ed4ae6a8054fd489dd011aeaf8609378b48 can you test if this repro's for you?

from goblin.

Sadly not, undoing the changes from the specified commit does not resolve the issue. I ran the code I provided again (ofc. with the changed version of goblin), and the results are still the same. The name is still offset by 4 characters.

from goblin.

hmmm, if i run bingrep on your exe, all the libraries functions it imports seem ok, so i don't think this is a general problem in our strtab parsing itself.

In your example code could you try using the name_offset() api on Symbol? e.g. change this line fromlet strtab_offset = u32::from_le_bytes(sym.name[4..8].try_into().unwrap()); to use let stratb_offset = sym.name_offset().unwrap()

from goblin.

I suspect you needed this portion: https://github.com//m4b/goblin/blob/17a5c7cc992220cd72e349eddea99148f31c5e65/src/pe/symbol.rs#L240

from goblin.

In your example code could you try using the name_offset() api on Symbol? e.g. change this line fromlet strtab_offset = u32::from_le_bytes(sym.name[4..8].try_into().unwrap()); to use let stratb_offset = sym.name_offset().unwrap()

I needed to change your suggested fix a bit (since the unwrap() would not work in the current context. But this is no meaningful change):

let strtab_offset = match sym.name_offset() {
                    Some(val) => val,
                    None => continue,
                };

The Problem still persists when testing the "fixed" code with my local version of goblin (the one with git revert eaba4ed4ae6a8054fd489dd011aeaf8609378b48 ).

I also checked how the code would behave with the current release of goblin 0.7.1. It crashes with attempt to subtract with overflow on the exact line you already suggested (src\pe\symbol.rs:240:36).

Running in release seems to "fix" the issue for version 0.7.1

Matching the returned Option from get_at() and just continuing on a None value "resolves" the issue.

let name = match named_sym {
            (Some(name), _) => name,
            (None, sym) => {
                let strtab_offset = match sym.name_offset() {
                    Some(val) => val,
                    None => continue,
                };
                match strtab.get_at(strtab_offset as usize) {
                    Some(name) => name,
                    None => continue,
                }
            }
        };

But this "fix" seems highly suspicious to me. First of all, there should not be a difference when running debug and release (regarding the code in question). Second, I just printed all symbols found by this code and there are some symbols that can't be found with objdump -t <.exe> | grep <sym_name>. I assume this might be due to the overflowing subtraction.

Based on this, I still think that there might be some deeply rooted problem within goblins PE module...

from goblin.

This is wrong because it will try to parse auxiliary symbols as regular symbols. Use symbols.iter() instead.

True. It seems to be working now.

Thank you for your help @m4b and @philipc !

(I will leave closing the issue up to you, due to the checked_sub code change)

from goblin.